Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --only option to sharezone cli. #1292

Merged
merged 1 commit into from
Feb 8, 2024
Merged

Add --only option to sharezone cli. #1292

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion tools/sz_repo_cli/lib/src/common/src/concurrent_command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ import 'package:sz_repo_cli/src/common/common.dart';
abstract class ConcurrentCommand extends CommandBase {
ConcurrentCommand(super.context) {
argParser
..addOption(
'only',
help:
'Only run the task for the given package(s). Package names can be separated by comma. E.g. `--only package1` or `--only=package1,package2`.',
)
Comment on lines +18 to +22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure the description for the --only option is clear and concise. Consider mentioning the expected behavior when no package names are provided.

..addConcurrencyOption(defaultMaxConcurrency: defaultMaxConcurrency)
..addPackageTimeoutOption(
defaultInMinutes: defaultPackageTimeout.inMinutes);
Expand Down Expand Up @@ -54,7 +59,25 @@ abstract class ConcurrentCommand extends CommandBase {
///
/// Can be overridden to e.g. add a filter (`.where((package) =>
/// package.hasTestDirectory`).
Stream<Package> get packagesToProcess => repo.streamPackages();
Stream<Package> get packagesToProcess {
var stream = repo.streamPackages();

final onlyPackageNames = _parseOnlyArg();
if (onlyPackageNames.isNotEmpty) {
stream =
stream.where((package) => onlyPackageNames.contains(package.name));
}

return stream;
}
Comment on lines +62 to +72
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of filtering packages based on the --only option in packagesToProcess is correct. However, consider the performance impact of calling stream.where inside an if statement. For large repositories, this could introduce a delay. Evaluate if there's a more efficient way to filter the packages, possibly by pre-processing the list before converting it to a stream.


List<String> _parseOnlyArg() {
final onlyArg = argResults!['only'] as String?;
if (onlyArg == null) {
return [];
}
return onlyArg.split(',');
}
Comment on lines +74 to +80
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _parseOnlyArg method correctly parses the --only argument. However, it does not trim whitespace from the package names after splitting. This could lead to unexpected behavior if users include spaces after commas. Add trimming to ensure robustness.

- return onlyArg.split(',');
+ return onlyArg.split(',').map((name) => name.trim()).toList();
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
List<String> _parseOnlyArg() {
final onlyArg = argResults!['only'] as String?;
if (onlyArg == null) {
return [];
}
return onlyArg.split(',');
}
List<String> _parseOnlyArg() {
final onlyArg = argResults!['only'] as String?;
if (onlyArg == null) {
return [];
}
return onlyArg.split(',').map((name) => name.trim()).toList();
}


@override
Future<void> run() async {
Expand Down
Loading