Repository navigation
Pub dependencies project validator - #106895
auto-submit[bot] merged 10 commits into
Conversation
flutter-dashboard
Bot
added
the
tool
| group('PubDependenciesProjectValidator', () { | ||
|
|
||
| setUp(() { | ||
| fileSystem = globals.localFileSystem; |
There was a problem hiding this comment.
does this need to use the real file system? and if so, this should probably be in the integration shard.
There was a problem hiding this comment.
Not really. I can use a FakeFileSystem here
| fileSystem = globals.localFileSystem; | ||
| }); | ||
|
|
||
| testWithoutContext('success ', () async { |
There was a problem hiding this comment.
out of curiosity, why the trailing space in the test name?
There was a problem hiding this comment.
That was a typo
| fileSystem = globals.localFileSystem; | ||
| }); | ||
|
|
||
| testWithoutContext('success ', () async { |
There was a problem hiding this comment.
also, maybe name this 'success when all dependencies are hosted'
| final ProcessManager processManager = FakeProcessManager.list(<FakeCommand>[ | ||
| const FakeCommand( | ||
| command: <String>['dart', 'pub', 'deps', '--json'], | ||
| stdout: 'stdout command fail', |
There was a problem hiding this comment.
what does this mean?
There was a problem hiding this comment.
Just expecting something not a json (eg: if you haven't updated your pub dependencies and the pubspec has changed pub dart deps returns a message to update them. I don't have the exact message so I just fake this)
There was a problem hiding this comment.
Also if for some reason dart pub deps --json breaks we will catch that and pass the stacktrace to the validator output
There was a problem hiding this comment.
Good thinking. However, if the command breaks any debug messaging should go to STDERR, right?
There was a problem hiding this comment.
I could be wrong, but I would imagine if you pass --json, only valid JSON should be output to stdout, and any diagnostics would go to stderr.
| final List<ProjectValidatorResult> result = await validator.start( | ||
| FlutterProject.fromDirectoryTest(fileSystem.currentDirectory) | ||
| ); | ||
| const String expected = 'All dependencies are hosted'; |
There was a problem hiding this comment.
will the user have enough context to understand what this means?
There was a problem hiding this comment.
The user will read [✓] Dart dependencies: All dependencies are hosted
There was a problem hiding this comment.
will they know what that means?
There was a problem hiding this comment.
I don't know. Should I add something like [✓] Dart dependencies: All dart dependencies comes from known places or something along those lines?
There was a problem hiding this comment.
will they know what "comes from" means?
I guess backing up, what is the high level information that you want to communicate to the user?
There was a problem hiding this comment.
If one ore more dependencies do not come from pub.dev eg: they are pointing to a github repo to get it #103614
| final List<ProjectValidatorResult> result = await validator.start( | ||
| FlutterProject.fromDirectoryTest(fileSystem.currentDirectory) | ||
| ); | ||
| const String expected = 'dep1, dep2 are not hosted'; |
There was a problem hiding this comment.
same question, will the user have enough context to understand what this means? in other words, are we going to get bug reports that they're getting warnings on their project when everything works?
|
|
||
| final DartPubJson dartPubJson = DartPubJson(jsonResult); | ||
| final List <String> dependencies = <String>[]; | ||
| final Set<String> hostedDependencies = {'hosted', 'root'}; |
There was a problem hiding this comment.
what does 'root' mean? please add a comment explaining both hosted and root.
| } | ||
|
|
||
| class PubDependenciesProjectValidator extends ProjectValidator { | ||
| PubDependenciesProjectValidator(this._processManager); |
There was a problem hiding this comment.
| PubDependenciesProjectValidator(this._processManager); | |
| const PubDependenciesProjectValidator(this._processManager); |
There was a problem hiding this comment.
I couldn't implement this because of the abstract class
There was a problem hiding this comment.
Can you make the super also const?
There was a problem hiding this comment.
you'll have to make title final, but i don't think that should be a problem
| @@ -0,0 +1,62 @@ | |||
| import 'dart:collection'; | |||
|
|
|||
| class Package { | |||
There was a problem hiding this comment.
this definitely needs a dartdoc, and maybe a more specific name. Note that since the convention in Dart is to just do import 'foo.dart';, every name gets imported into that library's namespace
| if (items.isEmpty || items.length > 1) { | ||
| throwToolExit('The suggestions flags needs one directory path'); | ||
| if (items.isEmpty) { // user did not specify any path | ||
| _logger.printStatus('Showing suggestions for current directory'); |
There was a problem hiding this comment.
| _logger.printStatus('Showing suggestions for current directory'); | |
| _logger.printTrace('Showing suggestions for current directory: ${_fileSystem.currentDirectory.path}'); |
| final List<String> dependencies; | ||
|
|
||
| static Package fromHashMap(dynamic packageInfo) { | ||
| static DartDependencyPackage fromHashMap(dynamic packageInfo) { |
There was a problem hiding this comment.
Dart style nit:
| static DartDependencyPackage fromHashMap(dynamic packageInfo) { | |
| factory DartDependencyPackage.fromHashMap(dynamic packageInfo) { |
There was a problem hiding this comment.
this is a style nit, this could very well compile to the same machine code
| const String expected = '\n' | ||
| '┌────────────────────────────────────────────────────────────────────────────────────┐\n' | ||
| '│ Pub dependencies │\n' | ||
| '│ [✓] Dart dependencies: All pub dependencies are hosted in https://pub.dartlang.org │\n' |
There was a problem hiding this comment.
| '│ [✓] Dart dependencies: All pub dependencies are hosted in https://pub.dartlang.org │\n' | |
| '│ [✓] Dart dependencies: All pub dependencies are hosted on https://pub.dartlang.org │\n' |
|
nit: let's just make these all green, success, since it's not really a problem. Follow-up will be in #107675 |
Jasguerrero
added
the
autosubmit
2 participants
#2885
Pre-launch Checklist
///).