fix: better error handling for release with no platforms provided #2264
Conversation
|
|
||
| verify( | ||
| () => logger.err( | ||
| 'At least one platform must be specified.', |
There was a problem hiding this comment.
Is this all the user sees? Will they know what to do with that or how to specify a platform?
There was a problem hiding this comment.
yeah, that is the only error message that I implemented, open to suggestions on a better message :)
There was a problem hiding this comment.
@eseidel I think they will known what to do as this is probably a often used command and they just did forgot by mistake to add the platforms.
But we could eventually make this message something like:
No platforms were provided, use the --platforms argument to provide one or more platforms
Wdyt?
There was a problem hiding this comment.
I agree with @eseidel here—we should endeavor to tell the user how to fix their problems when we can.
There was a problem hiding this comment.
No platforms were provided, use the --platforms argument to provide one or more platforms
Let's use this.
There was a problem hiding this comment.
That is fair! I've changed, can you take another look please?
packages/shorebird_cli/test/src/commands/release/release_command_test.dart
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2264 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 186 186
Lines 5862 5866 +4
=========================================
+ Hits 5862 5866 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| if (releaseTypeCliNames == null) { | ||
| return const []; | ||
| } |
There was a problem hiding this comment.
I don't think we need this early return. map on an empty list will return an empty list
There was a problem hiding this comment.
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!
Description
Adds a better error handling for the release command when no platforms were provided.
Fixes #2261
Type of Change