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

fix: better error handling for release with no platforms provided #2264

Merged
merged 5 commits into from
Jun 18, 2024
Merged

fix: better error handling for release with no platforms provided #2264

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
adf6a7a
fix: better error handling for release with no platforms informed
erickzanardo Jun 18, 2024
63d7ce2
better approach
erickzanardo Jun 18, 2024
133c73f
improving message
erickzanardo Jun 18, 2024
2429d4e
Merge branch 'main' into fix/no-platform-arguments
erickzanardo Jun 18, 2024
55973b9
tweaking code style
erickzanardo Jun 18, 2024
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
7 changes: 7 additions & 0 deletions packages/shorebird_cli/lib/src/commands/release/release_command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,13 @@ of the iOS app that is using this module. (aar and ios-framework only)''',

@override
Future<int> run() async {
if (results.releaseTypes.isEmpty) {
logger.err(
'''No platforms were provided, use the --platforms argument to provide one or more platforms''',
);
return ExitCode.usage.code;
}

final releaserFutures =
results.releaseTypes.map(_resolveReleaser).map(createRelease);

Expand Down
18 changes: 12 additions & 6 deletions packages/shorebird_cli/lib/src/release_type.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,22 @@ enum ReleaseType {

extension ReleaseTypeArgs on ArgResults {
Iterable<ReleaseType> get releaseTypes {
final List<String> releaseTypeCliNames;
List<String>? releaseTypeCliNames;
if (wasParsed('platforms')) {
releaseTypeCliNames = this['platforms'] as List<String>;
} else {
final platformCliName = arguments.first;
if (ReleaseType.values
.none((target) => target.cliName == platformCliName)) {
throw ArgumentError('Invalid platform: $platformCliName');
if (arguments.isNotEmpty) {
final platformCliName = arguments.first;
if (ReleaseType.values
.none((target) => target.cliName == platformCliName)) {
throw ArgumentError('Invalid platform: $platformCliName');
}
releaseTypeCliNames = [platformCliName];
}
releaseTypeCliNames = [platformCliName];
}

if (releaseTypeCliNames == null) {
return const [];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this early return. map on an empty list will return an empty list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, but it is not checking on an empty list, it is checking on nullable map, we need an early return.

Either way, your comment made me think and I changed the early return for an elvis operator and a coalesce to empty list directly on the return and I guess it looks better!


return releaseTypeCliNames.map(
Expand Down
4 changes: 2 additions & 2 deletions packages/shorebird_cli/pubspec.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,10 +330,10 @@ packages:
dependency: transitive
description:
name: http_parser
sha256: "2aa08ce0341cc9b354a498388e30986515406668dbcc4f7c950c3e715496693b"
sha256: "40f592dd352890c3b60fec1b68e786cefb9603e05ff303dbc4dda49b304ecdf4"
url: "https://pub.dev"
source: hosted
version: "4.0.2"
version: "4.1.0"
intl:
dependency: "direct main"
description:
Expand Down
18 changes: 18 additions & 0 deletions packages/shorebird_cli/test/src/commands/release/release_command_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,5 +521,23 @@ $exception''',
});
});
});

group('when no platform argument is provided', () {
setUp(() {
when(() => argResults['platforms']).thenReturn(const <String>[]);
});

test('fails and log the correct message', () async {
final exitCode = await runWithOverrides(command.run);

expect(exitCode, equals(ExitCode.usage.code));

verify(
() => logger.err(
'''No platforms were provided, use the --platforms argument to provide one or more platforms''',
),
).called(1);
});
});
});
}
59 changes: 34 additions & 25 deletions packages/shorebird_cli/test/src/release_type_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ void main() {
);
});

group('when nothing is provided', () {
test('parses and return empty', () {
expect(
parser.parse([]).releaseTypes.toList(),
isEmpty,
);
});
});

group('when the platforms argument is provided', () {
test('parses the release types', () {
expect(
Expand All @@ -51,33 +60,33 @@ void main() {
[ReleaseType.aar],
);
});
});

group('when the platforms is provided as a raw arg', () {
test('throws an ArgumentError if the platform is invalid', () {
expect(
() => parser.parse(['foo']).releaseTypes.toList(),
throwsArgumentError,
);
});
group('when the platforms is provided as a raw arg', () {
test('throws an ArgumentError if the platform is invalid', () {
expect(
() => parser.parse(['foo']).releaseTypes.toList(),
throwsArgumentError,
);
});

test('parses the release types', () {
expect(
parser.parse(['android', 'foo']).releaseTypes.toList(),
[ReleaseType.android],
);
expect(
parser.parse(['ios', 'foo']).releaseTypes.toList(),
[ReleaseType.ios],
);
expect(
parser.parse(['ios-framework', 'foo']).releaseTypes.toList(),
[ReleaseType.iosFramework],
);
expect(
parser.parse(['aar', 'foo']).releaseTypes.toList(),
[ReleaseType.aar],
);
});
test('parses the release types', () {
expect(
parser.parse(['android', 'foo']).releaseTypes.toList(),
[ReleaseType.android],
);
expect(
parser.parse(['ios', 'foo']).releaseTypes.toList(),
[ReleaseType.ios],
);
expect(
parser.parse(['ios-framework', 'foo']).releaseTypes.toList(),
[ReleaseType.iosFramework],
);
expect(
parser.parse(['aar', 'foo']).releaseTypes.toList(),
[ReleaseType.aar],
);
});
});
});
Expand Down
Loading