-
Notifications
You must be signed in to change notification settings - Fork 121
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: patch multi dimensional flavor patch and improve release #1968
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1968 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 177 177
Lines 5560 5562 +2
=========================================
+ Hits 5560 5562 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
37e2b2f
to
11750f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple minor things, but LGTM!
final bundleDirPath = p.join( | ||
projectRoot.path, | ||
'build', | ||
'app', | ||
'outputs', | ||
'bundle', | ||
); | ||
final bundlePath = flavor != null | ||
? p.join(bundleDirPath, '${flavor}Release', 'app-$flavor-release.aab') | ||
: p.join(bundleDirPath, 'release', 'app-release.aab'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so nice
packages/shorebird_cli/test/src/commands/build/build_apk_command_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/lib/src/commands/patch/patch_android_command.dart
Show resolved
Hide resolved
@@ -171,21 +170,24 @@ Use `shorebird flutter versions list` to list available versions. | |||
'Building release with Flutter $flutterVersionString', | |||
); | |||
|
|||
late final File apkFile; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was late final
before (and not something that needs to be addressed in this PR), but probably should be nullable instead because it will never be assigned a value in the default shorebird release android
case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wondered about that same thing for a couple of minutes too, I guess using late final made this a bit hacky, since yes, there will be moments which this will be null. Maybe I should change it to be explicit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should eventually be. This existed before your PR, so I'm not sure you need to fix it here.
packages/shorebird_cli/lib/src/shorebird_android_artifacts.dart
Outdated
Show resolved
Hide resolved
Co-authored-by: Bryan Oltman <bryan@shorebird.dev>
Description
Type of Change