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.

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_cli): verify that validators can be run before running them #1254

Merged
merged 4 commits into from
Sep 11, 2023

Conversation

bryanoltman
Copy link
Contributor

@bryanoltman bryanoltman commented Sep 11, 2023

Description

Updates validatePreconditions to check that validators can be run before running them. If a validator that cannot be run in the current context is provided, prints an error message/usage instructions and exits.

Example:
Screenshot 2023-09-11 at 4 14 20 PM

Fixes #1253

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (d12faa7) 98.93% compared to head (778ec6c) 98.93%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1254   +/-   ##
=======================================
  Coverage   98.93%   98.93%           
=======================================
  Files         179      179           
  Lines        4586     4590    +4     
=======================================
+ Hits         4537     4541    +4     
  Misses         49       49           
Flag Coverage Δ
shorebird_cli 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...idators/android_internet_permission_validator.dart 100.00% <ø> (ø)
...ib/src/validators/shorebird_flutter_validator.dart 100.00% <ø> (ø)
...ib/src/validators/shorebird_version_validator.dart 100.00% <ø> (ø)
...ges/shorebird_cli/lib/src/shorebird_validator.dart 100.00% <100.00%> (ø)
...s/shorebird_cli/lib/src/validators/validators.dart 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@eseidel eseidel left a comment

Choose a reason for hiding this comment

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

It's unclear to me from reading this what the affect on the user is.

e.g. is this trying to communicate a programmer error (we're executing code we didn't mean to some place) or user error (you're trying to do an unsupported thing)?

Is this validator only ever run for shorebird release android? Or is it also run for shorebird doctor?

Relatedly, do we handle using shorebird without an android directory at all? e.g. if you just wanted to use Shorebird on iOS?

@bryanoltman
Copy link
Contributor Author

@eseidel added an example screenshot to the description.

It's unclear to me from reading this what the affect on the user is.

e.g. is this trying to communicate a programmer error (we're executing code we didn't mean to some place) or user error (you're trying to do an unsupported thing)?

A helpful error message will be printed instead of a crash when a user runs an android command in a Flutter app/module directory that does not have android support.

Is this validator only ever run for shorebird release android? Or is it also run for shorebird doctor?

Both. shorebird doctor will only attempt to run it if canRunInContext is true.

Relatedly, do we handle using shorebird without an android directory at all? e.g. if you just wanted to use Shorebird on iOS?

Yes (or we should and it's a bug that we don't). This validator only is only run on android-specific commands, so it should not be invoked for anything iOS-related.

@bryanoltman bryanoltman merged commit ba1c06d into main Sep 11, 2023
@bryanoltman bryanoltman deleted the bo/validator-context branch September 11, 2023 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running shorebird release android in flutter module crashes
2 participants