[flutter_tools] General info project validator #103653
Conversation
flutter-dashboard
bot
added
the
tool
Jasguerrero
added
tool
| ]; | ||
| } | ||
|
|
||
| class GeneralInfoProjectValidator extends ProjectValidator{ |
There was a problem hiding this comment.
Can you add a dartdoc?
There was a problem hiding this comment.
These actually need to be /// for them to be parsed by the dartdoc tool: https://dart.dev/guides/language/effective-dart/documentation#do-use--doc-comments-to-document-members-and-types
There was a problem hiding this comment.
Also, this should be of the format: single line summary sentence, followed by empty line, then remainder: https://dart.dev/guides/language/effective-dart/documentation#do-separate-the-first-sentence-of-a-doc-comment-into-its-own-paragraph
| class GeneralInfoProjectValidator extends ProjectValidator{ | ||
| @override | ||
| Future<List<ProjectValidatorResult>> start(FlutterProject project) async { | ||
| final YamlMap pubContent = loadYaml(project.pubspecFile.readAsStringSync()) as YamlMap; |
There was a problem hiding this comment.
Do we not have elsewhere in the codebase where we parse a project's pubspec? If not, I would recommend pulling this out into its own file. Parsing Yaml is tricky and error prone, we should ideally have a single source code file that knows about the structure of pubspec.yaml and does all the type checking once.
There was a problem hiding this comment.
In fact, it would be nice if this was a class, that exposed getters for all the fields that we need to read.
There was a problem hiding this comment.
There is a general package for parsing pubspec but is missing stuff that I need like the flutter key. Agree on making this it's own class
There was a problem hiding this comment.
Yeah, also we should generally try not to add any more pub dependencies to this repo. We already have a lot of trouble pub solving.
|
|
||
| String getSupportedPlatforms(FlutterProject project) { | ||
| final List<SupportedPlatform> supportedPlatforms = project.getSupportedPlatforms(); | ||
| final List<String> allPlatforms = <String>[]; |
There was a problem hiding this comment.
This can be:
| final List<String> allPlatforms = <String>[]; | |
| final List<String> allPlatforms = project.getSupportedPlatforms().map((SupportedPlatform platform) => platform.name); |
| return content['flutter'] as YamlMap; | ||
| } | ||
|
|
||
| bool isFlutterPackage() { |
There was a problem hiding this comment.
style nit: this can be a getter, since it doesn't accept any parameters. All call-sites will need to be updated to omit the empty parentheses.
| bool isFlutterPackage() { | |
| bool get isFlutterPackage { |
| return flutterNode != null; | ||
| } | ||
|
|
||
| bool usesMaterialDesign() { |
There was a problem hiding this comment.
| bool usesMaterialDesign() { | |
| bool get usesMaterialDesign { |
| return flutterNode['uses-material-design'] as bool; | ||
| } | ||
|
|
||
| bool isPlugin() { |
There was a problem hiding this comment.
| bool isPlugin() { | |
| bool get isPlugin { |
| import 'package:yaml/yaml.dart'; | ||
|
|
||
| class PubContent { | ||
| PubContent(this.content); |
There was a problem hiding this comment.
nit, this constructor can be const. for the real tool, we probably won't be able to make any instances const (since we will be reading the content from disk), but maybe in tests?
| PubContent(this.content); | |
| const PubContent(this.content); |
|
|
||
| import 'package:yaml/yaml.dart'; | ||
|
|
||
| class PubContent { |
There was a problem hiding this comment.
nit. let's call this PubspecContent, since pub is the name of the tool. Also, can you name this file pubspec_content.dart?
| class PubContent { | |
| class PubspecContent { |
|
Started reviewing this, but then I found https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/flutter_manifest.dart. Let's whether it's worth de-duping logic next week |
I think I can use this class but I still believe I should make wrappers for its methods inside my current |
We could update the dartdoc text, right :) ? I'm not saying I'm sure adding new features to the |
Not really an specific reason not to do it just didn't want to add stuff that wasn't meant to be on that class but if we good on changing that dartdoc and add more things as need I can use this class directly for everything related to the pubspec.yml @christopherfujino |
It's unfortunate the class is named |
| ]; | ||
| } | ||
|
|
||
| /// Validator run for all platforms that extract information from the pubspec.yaml |
There was a problem hiding this comment.
| /// Validator run for all platforms that extract information from the pubspec.yaml | |
| /// Validator run for all platforms that extract information from the pubspec.yaml. |
|
|
||
| /// Validator run for all platforms that extract information from the pubspec.yaml | ||
| /// | ||
| /// Specific info from different platforms should be written in their own ProjectValidator |
There was a problem hiding this comment.
| /// Specific info from different platforms should be written in their own ProjectValidator | |
| /// Specific info from different platforms should be written in their own ProjectValidator. |
| class GeneralInfoProjectValidator extends ProjectValidator{ | ||
| @override | ||
| Future<List<ProjectValidatorResult>> start(FlutterProject project, {required Logger logger, required FileSystem fileSystem}) async { | ||
| final FlutterManifest? flutterManifest = FlutterManifest.createFromPath( |
There was a problem hiding this comment.
I think the project already has a getter for a FlutterManifest https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/project.dart#L126.
If so, I think this method wouldn't need to take a FileSystem or a Logger
| final ProjectValidatorResult appNameValidatorResult = getAppNameResult(flutterManifest); | ||
| final String supportedPlatforms = getSupportedPlatforms(project); | ||
| if (supportedPlatforms.isEmpty) { | ||
| return <ProjectValidatorResult>[appNameValidatorResult]; |
There was a problem hiding this comment.
Rather than returning a list literal, can you create an empty list at the start of this function, adding to it as you go, and if we need to return early, return that reference. This will be more resilient in the future when we add more validators, we won't have to update all subsequent list literals.
| } | ||
|
|
||
| ProjectValidatorResult isFlutterPackageValidatorResult(FlutterManifest flutterManifest) { | ||
| String value; |
There was a problem hiding this comment.
These two can be final
| ); | ||
| } | ||
|
|
||
| ProjectValidatorResult isFlutterPackageValidatorResult(FlutterManifest flutterManifest) { |
There was a problem hiding this comment.
please mark @visibleForTesting if used from tests, otherwise private.
|
This pull request is not suitable for automatic merging in its current state.
|
|
This pull request is not suitable for automatic merging in its current state.
|
|
This pull request is not suitable for automatic merging in its current state.
|
|
This pull request is not suitable for automatic merging in its current state.
|
#2885
Pre-launch Checklist
///).