[flutter_tool] New command project#102118
Conversation
There was a problem hiding this comment.
nit. .toString() is unneeded, the default behavior of interpolating a non-string into a string is to call .toString() on it anyway
| buffer.write('$icon ${result.toString()}'); | |
| buffer.write('$icon $result'); |
There was a problem hiding this comment.
why is this needed?
There was a problem hiding this comment.
No need. will delete it next commit
There was a problem hiding this comment.
nit. prefer final and you can replace the if/else with a ternary
There was a problem hiding this comment.
this can be final
There was a problem hiding this comment.
Shouldn't you be printing the buffer outside of the for loop?
If this is a bug, can you write a test that would have caught it (for example, hard-code the expected logger output, so that we can see what it should look like)? Add it to packages/flutter_tools/test/commands.shard/hermetic/analyze_project_test.dart
There was a problem hiding this comment.
Yes. It should be outside, adding a test is a good idea
There was a problem hiding this comment.
might as well make this public and get rid of the getter.
There was a problem hiding this comment.
nit. prefer named parameters when you have more than 2
There was a problem hiding this comment.
also, this constructor can be made const
There was a problem hiding this comment.
| // add validators | |
| // TODO(jsguerrero): add validators |
There was a problem hiding this comment.
Can you add dartdoc's for all the new classes you add, and any non-obvious methods? (for example .supportsProject is self-explanatory)
There was a problem hiding this comment.
nit. make this a static on the ProjectValidator class. Static's are essentially global like this; however, it's a little more organized.
There was a problem hiding this comment.
nit use named parameters when you have more than 2
There was a problem hiding this comment.
We should be consistent with the doctor and use this unicode character: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/doctor_validator.dart#L258
There was a problem hiding this comment.
if you call buffer.writeln() here you don't need to print an extra newline in the above scope (line 53).
| buffer.write('$icon $result'); | |
| buffer.writeln('$icon $result'); |
There was a problem hiding this comment.
if you are returning early, you can un-nest the conditionals to make reading this easier:
if (_status == StatusProjectValidator.error) {
return 'Error: $value';
}
if (warning != null) {
return '$name: $value (warning: $warning)';
}
return '$name: $value';There was a problem hiding this comment.
do we need to await one before starting the next, or are these safe to run concurrently?
There was a problem hiding this comment.
Also, can we use a spinner like in the doctor, so that the user knows the tool is still running?
https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/doctor.dart#L341
There was a problem hiding this comment.
Also maybe we should catch exceptions and mark them as crashed, as we do in the doctor: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/doctor.dart#L350
There was a problem hiding this comment.
i think you can mark this as final
| String icon; | |
| final String icon; |
christopherfujino
left a comment
There was a problem hiding this comment.
This is coming along great. Just a few nits.
There was a problem hiding this comment.
Don't re-use the same filesystem between tests
| setUpAll(() { | |
| setUp(() { |
There was a problem hiding this comment.
I'm thinking you probably shouldn't re-use the logger between tests?
There was a problem hiding this comment.
Can we get rid of this extra line?
There was a problem hiding this comment.
| static List <ProjectValidator> allProjectValidators = <ProjectValidator>[ | |
| static const List <ProjectValidator> allProjectValidators = <ProjectValidator>[ |
packages/flutter_tools/test/commands.shard/hermetic/project_validator_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/commands.shard/hermetic/project_validator_test.dart
Outdated
Show resolved
Hide resolved
ccc923e to
e0e74b6
Compare
| terminal: globals.terminal, | ||
| artifacts: globals.artifacts, | ||
| ), | ||
| ValidateProjectCommand( |
There was a problem hiding this comment.
I don't think we should add this until it's ready to be used.
|
This pull request is not suitable for automatic merging in its current state.
|
#2885
Pre-launch Checklist
///).