[tool] refactor publish plugin command #3779
Conversation
| argParser.addOption( | ||
| _allOption, | ||
| help: | ||
| 'Based the diff between $_kBaseShaOption and HEAD, publish all the plugins that has pubspec change in this diff.\n' |
There was a problem hiding this comment.
Nit: "have pubspec changes"
| _allOption, | ||
| help: | ||
| 'Based the diff between $_kBaseShaOption and HEAD, publish all the plugins that has pubspec change in this diff.\n' | ||
| 'The --package option is ignored if this flag is on. (This is currently under construction)' , |
There was a problem hiding this comment.
What does "under construction" mean here? Is the plan for this behavior to change?
There was a problem hiding this comment.
It meant it is not supported yet. I had everything wired up in a different branch but i thought to introduce this pr as an intermediate step is easier to review.
I should just remove the flag int this PR as well and add this flag when actually adding this feature.
| argParser.addOption( | ||
| _allOption, | ||
| help: | ||
| 'Based the diff between $_kBaseShaOption and HEAD, publish all the plugins that has pubspec change in this diff.\n' |
There was a problem hiding this comment.
Do we need to handle the case where there have been multiple releases of a single plugin in the range, or are we going to avoid that issue in automation by never doing a range spanning multiple changes?
There was a problem hiding this comment.
The issue won't exist when we start the automate process, because every commit that contains a pubspec change is going to run a publish command.
There might be leftovers before the automation so we might need to handle that, handle it manually before automation doesn't seem to be too bad.
| _print('Package published!'); | ||
| } | ||
|
|
||
| Future<void> _tagRelease({@required Directory packageDir, @required String remote, @required String remoteUrl, @required bool shouldPushTag}) async { | ||
| if (!argResults[_tagReleaseOption]) { |
There was a problem hiding this comment.
I think it would make more sense to wrap the call to _tagRelease in this check, rather than making the function silently no-op based on an argument.
| final String tag = _getTag(); | ||
| await processRunner.runAndExitOnError('git', <String>['tag', tag], | ||
| workingDir: _packageDir); | ||
| final String tag = _getTag(packageDir); |
There was a problem hiding this comment.
Let's flip this and the line above, so the print can include the tag for ease of debugging issues.
| if (package == null) { | ||
| _print( | ||
| 'Must specify a package to publish. See `plugin_tools help publish-plugin`.'); | ||
| Directory _checkPackageDir(String package) { |
There was a problem hiding this comment.
Can we rename this to something like _getPackageDir (and document that it throws if the dir doesn't exist)? I would expect a check function to either return a bool, or return nothing.
| argParser.addOption( | ||
| _allOption, | ||
| help: | ||
| 'Based the diff between $_kBaseShaOption and HEAD, publish all the plugins that has pubspec change in this diff.\n' |
There was a problem hiding this comment.
The issue won't exist when we start the automate process, because every commit that contains a pubspec change is going to run a publish command.
There might be leftovers before the automation so we might need to handle that, handle it manually before automation doesn't seem to be too bad.
| _allOption, | ||
| help: | ||
| 'Based the diff between $_kBaseShaOption and HEAD, publish all the plugins that has pubspec change in this diff.\n' | ||
| 'The --package option is ignored if this flag is on. (This is currently under construction)' , |
There was a problem hiding this comment.
It meant it is not supported yet. I had everything wired up in a different branch but i thought to introduce this pr as an intermediate step is easier to review.
I should just remove the flag int this PR as well and add this flag when actually adding this feature.
cyanglaz
added
the
waiting for tree to go green
Refactoring publish plugin command to get ready for a new feature to publish and tag release based on git diff.
See #3776 (comment)
behavioral change: The command now validate the git remote first before checking package directory. Because when we change this command later to run multiple publishes, the git remote validation only needs to happen once.