fix: shorebird preview to identify shorebird projects with flavors #1854
Conversation
Co-authored-by: Bryan Oltman <bryan@shorebird.dev>
Co-authored-by: Bryan Oltman <bryan@shorebird.dev>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1854 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 173 173
Lines 5486 5489 +3
=========================================
+ Hits 5486 5489 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| Future<String?> promptForFlavor( | ||
| List<String> flavors, | ||
| ) async { | ||
| return logger.chooseOne<String>( | ||
| 'Which app flavor?', | ||
| choices: flavors, | ||
| ); | ||
| } |
There was a problem hiding this comment.
I'm not sure we gain much by wrapping this in its own function, as the call to logger.chooseOne is not substantially more complex than the call to promptForFlavor.
| () => logger.chooseOne<String>( | ||
| any(), | ||
| choices: any(named: 'choices'), |
There was a problem hiding this comment.
We should probably verify that the flavors were provided as arguments to chooseOne
There was a problem hiding this comment.
Fair enought! I've changed it
There was a problem hiding this comment.
Did you push? I'm still seeing:
verify(
() => logger.chooseOne<String>(
any(),There was a problem hiding this comment.
I pushed yes! That is weird, I am seeing it correctly as shown in the print above
There was a problem hiding this comment.
Ah! The issue is just that the test only has a single flavor. We should probably update that (and maybe change the command to not prompt if only one flavor is present, although that seems like sort of a silly setup)
There was a problem hiding this comment.
Oh, yeah, fair point!
Should we do that handling in a different PR though? I feel that this PR is addressing the detecting a shorebird project that has flavors, no matter the number, then auto selecting the only flavor would probably be a change of its own?
Let me know if you agree, otherwise I am happy to update this PR to include this as well.
There was a problem hiding this comment.
I think the "single flavor" fix can be done in a future PR (if ever), but the test should probably be updated to include more than one flavor
There was a problem hiding this comment.
Fair enough, I've updated it
|
Looks great, ship it! |
Description
Uses the flavor values at
shorebird.yamlto prompt for which flavor to use when usingshorebird previewFixes #1836
Type of Change