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: shorebird preview to identify shorebird projects with flavors #1854

Merged
merged 6 commits into from
Apr 3, 2024
+21 −10
Merged

fix: shorebird preview to identify shorebird projects with flavors #1854

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f25debf
fix: shorebird preview to indentify shorebird projects with flavors
erickzanardo Apr 3, 2024
a15d0af
Update packages/shorebird_cli/lib/src/commands/preview_command.dart
erickzanardo Apr 3, 2024
8c3cb66
Update packages/shorebird_cli/lib/src/commands/preview_command.dart
erickzanardo Apr 3, 2024
6efc52b
reverting pubspec.lock
erickzanardo Apr 3, 2024
2ea9b3e
PR suggestions
erickzanardo Apr 3, 2024
805b968
PR suggestions
erickzanardo Apr 3, 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
18 changes: 17 additions & 1 deletion packages/shorebird_cli/lib/src/commands/preview_command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,17 @@ class PreviewCommand extends ShorebirdCommand {

final shorebirdYaml = shorebirdEnv.getShorebirdYaml();
final String? appId;

final flavors = shorebirdYaml?.flavors;

if (results.wasParsed('app-id')) {
appId = results['app-id'] as String;
} else if (shorebirdYaml != null && shorebirdYaml.flavors == null) {
} else if (shorebirdYaml != null && flavors == null) {
appId = shorebirdYaml.appId;
} else if (shorebirdYaml != null && flavors != null) {
final flavorOptions = flavors.keys.toList();
final choosenFlavor = await promptForFlavor(flavorOptions);
appId = flavors[choosenFlavor];
} else {
appId = await promptForApp();
}
Expand Down Expand Up @@ -172,6 +179,15 @@ class PreviewCommand extends ShorebirdCommand {
return app.appId;
}

Future<String?> promptForFlavor(
List<String> flavors,
) async {
return logger.chooseOne<String>(
'Which app flavor?',
choices: flavors,
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


Future<String?> promptForReleaseVersion(List<Release> releases) async {
if (releases.isEmpty) return null;
final release = logger.chooseOne(
Expand Down
8 changes: 4 additions & 4 deletions packages/shorebird_cli/pubspec.lock
View file
Open in desktop
erickzanardo marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,10 @@ packages:
dependency: transitive
description:
name: built_value
sha256: fedde275e0a6b798c3296963c5cd224e3e1b55d0e478d5b7e65e6b540f363a0e
sha256: c7913a9737ee4007efedaffc968c049fd0f3d0e49109e778edc10de9426005cb
url: "https://pub.dev"
source: hosted
version: "8.9.1"
version: "8.9.2"
characters:
dependency: transitive
description:
Expand Down Expand Up @@ -802,10 +802,10 @@ packages:
dependency: transitive
description:
name: win32
sha256: "8cb58b45c47dcb42ab3651533626161d6b67a2921917d8d429791f76972b3480"
sha256: "0a989dc7ca2bb51eac91e8fd00851297cfffd641aa7538b165c62637ca0eaa4a"
url: "https://pub.dev"
source: hosted
version: "5.3.0"
version: "5.4.0"
xml:
dependency: "direct main"
description:
Expand Down
12 changes: 5 additions & 7 deletions packages/shorebird_cli/test/src/commands/preview_command_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,7 @@ channel: ${track.channel}
verifyNever(() => codePushClientWrapper.getApps());
});

test('prompts for app id when in shorebird project with flavors',
test('prompts for the flavor when in shorebird project with flavors',
() async {
when(() => shorebirdEnv.getShorebirdYaml()).thenReturn(
const ShorebirdYaml(
Expand All @@ -723,20 +723,18 @@ channel: ${track.channel}
when(() => argResults.wasParsed('app-id')).thenReturn(false);
when(() => argResults['app-id']).thenReturn(null);
when(
() => logger.chooseOne<AppMetadata>(
() => logger.chooseOne<String>(
any(),
choices: any(named: 'choices'),
display: any(named: 'display'),
),
).thenReturn(app);
).thenReturn('dev');

await runWithOverrides(command.run);

verify(
() => logger.chooseOne<AppMetadata>(
'Which app would you like to preview?',
() => logger.chooseOne<String>(
any(),
choices: any(named: 'choices'),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably verify that the flavors were provided as arguments to chooseOne

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enought! I've changed it

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you push? I'm still seeing:

 verify(
          () => logger.chooseOne<String>(
            any(),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-04-03 at 13 52 16

I pushed yes! That is weird, I am seeing it correctly as shown in the print above

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

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, 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I've updated it

display: any(named: 'display'),
),
).called(1);
});
Expand Down
Loading