[flutter_conductor] update dev/tools with release tool#69791
[flutter_conductor] update dev/tools with release tool#69791fluttergithubbot merged 9 commits intoflutter:masterfrom
Conversation
|
@jonahwilliams sorry that this is such a big diff. the behavior should be identical to the roll_dev script. I generalized a lot of the components into classes, especially Once this lands, the next step will be supporting interactive mode, and supporting applying cherry picks to releases. |
jonahwilliams
left a comment
There was a problem hiding this comment.
Haven't read through the changes to the existing roll dev script yet
| @@ -0,0 +1,20 @@ | |||
| // Copyright 2014 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
added the function stdoutToString here, is that cool?
There was a problem hiding this comment.
I just have an irrational fear of globals
dev/tools/lib/repository.dart
Outdated
|
|
||
| /// Lazily-loaded directory for the repository checkout. | ||
| /// | ||
| /// Cloning a repository is time-consuming, thus the actual repository is not |
There was a problem hiding this comment.
ensured to exist -> cloned?
There was a problem hiding this comment.
Technically, if you pass useExistingCheckout = true, it will not necessarily clone every time. Later, when the conductor runs in interactive mode, subsequent commands will operate on the same local repo.
That having been said, I agree "ensured to exist" is confusing. I'll think of a better way to word it.
There was a problem hiding this comment.
"cloned if it does not already exist"?
dev/tools/lib/repository.dart
Outdated
| /// Cloning a repository is time-consuming, thus the actual repository is not | ||
| /// ensured to exist until this getter is called. | ||
| Directory get checkoutDirectory { | ||
| if (_checkoutDirectory == null) { |
There was a problem hiding this comment.
you could un-nest this with an early return:
if (_checkoutDirectory != null) {
...
}
There was a problem hiding this comment.
i think rubocop (ruby's linter) favors as few returns as possible, but I agree with your comment that as few indents as possible is favorable.
| latest, | ||
| } | ||
|
|
||
| class Version { |
There was a problem hiding this comment.
This class could benefit from some docs or links to the existing version format docs
dev/tools/lib/version.dart
Outdated
| factory Version.fromString(String versionString) { | ||
| assert(versionString != null); | ||
|
|
||
| final RegExp stablePattern = RegExp(r'^(\d+)\.(\d+)\.(\d+)$'); |
jonahwilliams
left a comment
There was a problem hiding this comment.
roll dev was the only existing functionality, correct?
| final ProcessResult result = _run(args, workingDirectory); | ||
| if ((result.stderr as String).isEmpty && result.exitCode == 0) | ||
| return (result.stdout as String).trim(); | ||
| _reportFailureAndExit(args, workingDirectory, result, explanation); |
There was a problem hiding this comment.
Just a note, once this is opted into null safety you can give _reportFailureAndExit a return type of Never and then there will be no analyzer warning
dev/tools/lib/git.dart
Outdated
| @required String workingDirectory, | ||
| }) { | ||
| final ProcessResult result = _run(args, workingDirectory); | ||
| if ((result.stderr as String).isEmpty && result.exitCode == 0) |
There was a problem hiding this comment.
sometimes stderr is output without it implying a non-zero exit. Could be a diagnostic message
There was a problem hiding this comment.
good point, as I should know (this was legacy code fwiw :P )
| List<String> args, | ||
| String explanation, { | ||
| @required String workingDirectory, | ||
| }) { |
There was a problem hiding this comment.
Here and elsewhere, I would separate the cast from the method invocation. maybe a little helper method like:
String decode(dynamic value) => (value as String).trim();
but with a better name 😭
There was a problem hiding this comment.
I can do that. Is your concern line numbers in the stack trace, or is there a higher level code quality concern?
There was a problem hiding this comment.
Its a bit awkward to read with the cast inline. Another approach would be to use toString() instead since that is defined on Object/dynamic
There was a problem hiding this comment.
implemented String stdoutToString(dynamic input)
correct |
NOTE: this is a squashed version of #65968 to fix a version issue. See that PR commit history.
Description
This is a re-write of the
roll_dev.dartscript as a full CLI tool complete with integration tests. At this point, the behavior should be exactly the same asroll_dev.dart. I am putting out a PR at this point so that the actual tool behavior improvements can be reviewed independent of this code re-organization.