[flutter_plugin_tools] Restructure version-check #4111
Conversation
| @@ -100,6 +101,26 @@ abstract class PluginCommand extends Command<void> { | |||
| return _shardCount!; | |||
| } | |||
|
|
|||
| /// Returns the [GitDir] containing [packagesDir]. | |||
| Future<GitDir> getGitDir() async { | |||
There was a problem hiding this comment.
nit: This should be a property.
There was a problem hiding this comment.
Changed. I wasn't aware that properties could be async.
| /// | ||
| /// This can be mocked for testing. | ||
| final GitDir? gitDir; | ||
| GitDir? _gitDir; |
There was a problem hiding this comment.
This should be a late final non-null field, no?
There was a problem hiding this comment.
It's used as a cache, so cannot be late final.
|
|
||
| final List<String> badVersionChangePubspecs = <String>[]; | ||
| final Pubspec? pubspec = _tryParsePubspec(package); | ||
| Pubspec.parse(package.childFile('pubspec.yaml').readAsStringSync()); |
There was a problem hiding this comment.
Why are you reading a string then throwing it away? To force a lazy value? In the original code it used this value.
There was a problem hiding this comment.
I missed removing it when converting the codepath to using _tryParsePubspec. Removed. (Probably it was autoformatted with this as the RHS of an assignment at an interim refactoring stage, then I accidentally deleted the first half and the second half auto-formatted back as a pointless statement.)
| return errors; // No remaining checks make sense. | ||
| } | ||
|
|
||
| if (!await _checkVersionChange(package, pubspec: pubspec)) { |
There was a problem hiding this comment.
nit: The verb "check" doesn't suggest what the return value actually means. if (!await _isVersionChangeValid(package, pubspec: pubspec)) would be easier to verify when reading the code.
There was a problem hiding this comment.
Changed to _hasValidVersionChange (the argument is a package, not a version, so it has a change, rather than being a change).
| errors.add('Disallowed version change.'); | ||
| } | ||
|
|
||
| if (!(await _checkVersionsMatch(package, pubspec: pubspec))) { |
There was a problem hiding this comment.
Same comment about "check", how about _doVersionsMatch?
There was a problem hiding this comment.
Changed to _hasConsistentVersion (for similar reasons; the arguments are not version being compared).
| /// | ||
| /// [packageName] must be the actual name of the package as published (i.e., | ||
| /// the name from pubspec.yaml, not the on disk name if different.) | ||
| Future<Version?> _getPreviousVersionFromPub(String packageName) async { |
There was a problem hiding this comment.
nit: This should be a property too, if you think it requires special consideration since it is hitting the web, you can use a verb like fetch.
There was a problem hiding this comment.
Changed to fetch. However:
a) this is not in any way a property of the command, but an attributed of the provided package, and
b) properties can't take arguments.
| continue; | ||
| } | ||
| /// Returns the version of [package] from git at the base comparison hash. | ||
| Future<Version?> _getPreviousVersionFromGit( |
* upstream_v0_8_1+3: (99 commits) [image_picker] Fixed IOException when cache directory is removed (flutter#4117) [in_app_purchase] Fix app exceptions caused by missing App Store receipt (flutter#4096) Add Basic Junit Tests to some plugins (flutter#4108) [image_picker]Update example app (flutter#4103) [flutter_plugin_tools] Restructure version-check (flutter#4111) Split some Cirrus script steps (flutter#4112) [flutter_plugin_tools] Migrate java-test to new base command (flutter#4105) [flutter_plugin_tools] ignore flutter_plugin_tools when publishing (flutter#4110) [in_app_purchase] Add support for SKPaymentQueueDelegate and showPriceConsentIfNeeded (flutter#4085) [flutter_plugin_tools] release 0.3.0 (flutter#4109) Migrate command, add failure test, remove skip (flutter#4106) Don't install cocoapods; use the version in the image (flutter#4104) [flutter_plugin_tools] Migrate analyze to new base command (flutter#4084) Add release status badge to README (flutter#4102) Build all iOS example apps on current Flutter stable (flutter#4101) [url_launcher] Fix test button check for iOS 15 (flutter#4088) Update .ci.yaml documentation link (flutter#4090) [image_picker] Updated pickImage and pickVideo docs to expose the possible errors that can be thrown (flutter#4089) [flutter_plugin_tools] `publish-plugin` check against pub to determine if a release should happen (flutter#4068) [webview_flutter] Suppress iOS 9 deprecation warnings (flutter#4100) ... # Conflicts: # packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m
Combines the two different aspects of version-checking into a single looping structure based on plugins, using the new base command, rather than one operating on plugins as controlled by the usual flags and the other operating on a git list of changed files. Also simplifies the determination of the new version by simply checking the file, rather than querying git for the HEAD state of the file. Tests setup is simplified since we no longer need to set up nearly as much fake `git` output. Minor changes to base commands: - Move indentation up to PackageLoopingCommand so that it is consistent across commands - Add a new post-loop command for any cleanup, which is needed by version-check - Change the way the GitDir instance is managed by the base PluginCommand, so that it's always cached even when not overridden, to reduce duplicate work and code. Part of flutter/flutter#83413
Combines the two different aspects of version-checking into a single looping structure based on plugins, using the new base command, rather than one operating on plugins as controlled by the usual flags and the other operating on a git list of changed files. Also simplifies the determination of the new version by simply checking the file, rather than querying git for the HEAD state of the file. Tests setup is simplified since we no longer need to set up nearly as much fake `git` output. Minor changes to base commands: - Move indentation up to PackageLoopingCommand so that it is consistent across commands - Add a new post-loop command for any cleanup, which is needed by version-check - Change the way the GitDir instance is managed by the base PluginCommand, so that it's always cached even when not overridden, to reduce duplicate work and code. Part of flutter/flutter#83413
Combines the two different aspects of version-checking into a single looping structure based on plugins, using the new base command, rather than one operating on plugins as controlled by the usual flags and the other operating on a git list of changed files.
Also simplifies the determination of the new version by simply checking the file, rather than querying git for the HEAD state of the file. Tests setup is simplified since we no longer need to set up nearly as much fake
gitoutput.Minor changes to base commands:
Part of flutter/flutter#83413
Pre-launch Checklist
dart format.)[shared_preferences]///).