Align yaml and validateSkills dart api#130
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to the dart_skills_lint tool, including tilde expansion for paths, improved configuration loading, and better documentation regarding CLI vs. Dart Test usage. Key changes include moving path expansion logic to a utility file, refactoring validateSkillsInternal to handle default skill directories more robustly, and adding a new test suite to ensure the repository's own skills are validated. Feedback highlights a missing tilde expansion in the configuration parser, a discrepancy between documentation and the actual exception thrown, and a recommendation to use absolute paths for more reliable directory matching. Additionally, the reviewer noted that the new test suite may skip default rules by only passing configured rules, and suggested making the tilde expansion utility more robust to handle the base ~ path.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates dart_skills_lint to version 0.3.0, introducing several enhancements and refactorings. Key changes include exposing the ConfigParser.loadConfig() API, supporting tilde expansion (~/) in configuration paths, and refactoring fallback logic for skills directories into validateSkillsInternal. The rule precedence logic was also updated to ensure CLI flags correctly override configuration file settings. Feedback focuses on improving the robustness of path matching to avoid false positives with directory prefixes and ensuring consistent tilde expansion across the new public API.
| /// | ||
| /// If a [path] is explicitly provided and the file does not exist, this | ||
| /// method throws a [FileSystemException]. If no path is provided and the | ||
| /// default file is missing, it returns an empty [Configuration] (preserving |
There was a problem hiding this comment.
Get rid of "Preserving current behavior"
| /// [config] is the loaded configuration. | ||
| /// | ||
| /// Returns `true` if all validations passed (or if generating a baseline), `false` otherwise. | ||
| Future<bool> validateSkills({ |
There was a problem hiding this comment.
Document meaning of return value.
| ); | ||
|
|
||
| exitCode = success ? 0 : 1; | ||
| bool success; |
There was a problem hiding this comment.
explicitly set the value for readability
| ignoreFileOverride: ignoreFileOverride, | ||
| config: config, | ||
| ); | ||
| exitCode = success ? 0 : 1; |
There was a problem hiding this comment.
Do not use a ternary here.
| Configuration? config, | ||
| List<SkillRule> customRules = const [], | ||
| }) async { | ||
| var effectiveSkillDirPaths = List<String>.from(skillDirPaths); |
There was a problem hiding this comment.
break out the logic to get effectiveSkillDirPaths into a method with documentation around how the paths are joined.
| import 'package:path/path.dart' as p; | ||
| import 'package:test/test.dart'; | ||
|
|
||
| void main() { |
There was a problem hiding this comment.
There is a lot of code duplication in this file. Figure out what make sense to extract.
| resolved, | ||
| isEmpty, | ||
| reason: | ||
| 'Defaults are now handled by Validator, so resolveRules should return empty map when no overrides.', |
There was a problem hiding this comment.
"Defaults are now handled" This is an antipattern the meaning of "now" changes over time. Should this test be removed? I am not sure it is valuable.
| }); | ||
|
|
||
| test('config overrides defaults', () { | ||
| test('ignores config rules', () { |
There was a problem hiding this comment.
is this test still valuable?
| - Refactored fallback logic for skills directories to `validateSkillsInternal`. | ||
| - Added tests for path utilities and configuration loading. |
There was a problem hiding this comment.
Changelogs should be about information the users should care about. Internal logic reorganization should not be included.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates dart_skills_lint to version 0.3.0, introducing tilde expansion for paths, exposing the ConfigParser.loadConfig() API, and implementing a clear rule precedence hierarchy (Global Config, Path-Specific Config, and CLI Overrides). It also adds documentation regarding CLI versus Dart Test usage and establishes a policy against using relative temporal words in documentation. Review feedback highlights opportunities to improve rule and ignore file resolution for nested directory structures and suggests more robust error handling for YAML parsing failures to avoid silent fallbacks to default settings.
Fixes #85
Pre-launch Checklist
///).