[tool] Unify most validation commands#11641
[tool] Unify most validation commands#11641stuartmorgan-g wants to merge 18 commits intoflutter:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request consolidates several individual validation tools, such as pubspec-check, readme-check, and gradle-check, into a single unified validate command. It introduces the ValidateCommand class, updates CI scripts and documentation, and enables per-package configuration for excerpt validation via ci_config.yaml. Review feedback identifies opportunities to improve the robustness of YAML parsing by implementing explicit type checks before casting to YamlList, YamlMap, or String to prevent potential runtime exceptions.
| if (yaml == null) { | ||
| return []; | ||
| } | ||
| return (yaml as YamlList).toList().cast<String>(); |
There was a problem hiding this comment.
The cast to YamlList will throw a TypeError if the YAML content is not a list (e.g., if it's a map or a scalar). Since this is a utility function, it should handle invalid YAML structures more gracefully or provide a clearer error message.
if (yaml is! YamlList) {
return <String>[];
}
return yaml.toList().cast<String>();| return ''; | ||
| } | ||
| const minFlutterKey = 'min_flutter'; | ||
| final config = loadYaml(versionConfig.readAsStringSync()) as YamlMap?; |
There was a problem hiding this comment.
The cast as YamlMap? will throw a TypeError if the configuration file contains a YAML structure that is not a map (e.g., a list or a single string). It's safer to check the type before casting.
| final config = loadYaml(versionConfig.readAsStringSync()) as YamlMap?; | |
| final Object? yaml = loadYaml(versionConfig.readAsStringSync()); | |
| if (yaml != null && yaml is! YamlMap) { | |
| printError( | |
| '$_versionConfigFileName must be a map containing a "$minFlutterKey" entry', | |
| ); | |
| throw ToolExit(_exitCodeVersionConfigIssue); | |
| } | |
| final YamlMap? config = yaml as YamlMap?; |
| ); | ||
| throw ToolExit(_exitCodeVersionConfigIssue); | ||
| } | ||
| return (config[minFlutterKey] as String).trim(); |
There was a problem hiding this comment.
The cast as String will fail if the YAML value is parsed as a different type, such as a double (e.g., min_flutter: 3.35 without quotes). Using .toString() is more robust.
| return (config[minFlutterKey] as String).trim(); | |
| return config[minFlutterKey].toString().trim(); |
| @@ -22,46 +22,12 @@ tasks: | |||
| script: .ci/scripts/tool_runner.sh | |||
| args: ["license-check"] | |||
There was a problem hiding this comment.
The reason this isn't combined is that it's a repo-wide command, not package-level (because we don't want files to accidentally not get license blocks when they aren't in a package).
It is annoying for local validation that it's not part of the same command, but I haven't thought of a good way to combine it yet without making it more likely to miss files.
| required path.Context path, | ||
| required String indentation, | ||
| required void Function(String) warningLogger, | ||
| required bool requireCodeExcerpts, |
There was a problem hiding this comment.
The changes in this file are to switch to the new package-local config.
| }) async { | ||
| final Pubspec pubspec = package.parsePubspec(); | ||
| final Pubspec? pubspec = _tryParsePubspec(package); | ||
| if (pubspec == null) { |
There was a problem hiding this comment.
I moved this logic from the command to the validator, since we no longer need to support "skip" for publish-to none, just pass.
| bool _shouldRun(Validator validator) => | ||
| targetedValidators?.contains(validator) ?? true; | ||
|
|
||
| /// Runs repo-level checks. |
There was a problem hiding this comment.
Essentially all of the code from here on in this file is moved with minimal changes from the deleted command files.
1 participant
Replaces all of the following repo tooling commands with a new unified
validatecommand:dependabot-checkgradle-checkpubspec-checkreadme-checkversion-checkIt also makes some simplifications in using these:
tool_config/directory. This avoids the need to pass a flag every time for something that isn't actually intended to be variable.tool_config/directory, instead of having to pass them in all the time, to make the commands easier to call. As with min SDK version, this isn't something callers actually need to be able to change on the fly to the flags add more complexity (in exchange for making it extremely clear at the CI call site what was happening, but I no longer think that tradeoff is the right one based on experience using the tool).To avoid huge amounts of test file churn, and massively complicating test setup, there is a test-only mechanism to select individual validators to run in tests, rather than having to test the entire command at once.
Part of flutter/flutter#173413