feat: support configurable rule-specific options and custom exclusions in path-does-not-exist#176
feat: support configurable rule-specific options and custom exclusions in path-does-not-exist#176reidbaker-agent wants to merge 10 commits into
Conversation
…pace and restructuring yaml configuration
- Rename Validator parameter ruleOverrides to customRuleSeverities. - Rename severity mapping fields and parameters to ruleSeverities. - Rename option mapping fields and parameters to ruleOptions. - Added validation checks for options keys and types in YAML parsing. - Decoupled and exported ValidationResult model.
… wrapper - Extract options validation to ConfigurableSkillRule base constructor. - Introduce CustomRuleOptions class wrapping options configuration maps. - Refactor PathDoesNotExistRule to extend ConfigurableSkillRule.
…tions - Centralize options schema validation and formatting in CheckType.validateOptions. - Remove redundant allowedOptions getter from ConfigurableSkillRule and its subclasses. - Update ConfigParser, entrypoint, Validator, and ValidationSession to use CustomRuleOptions instead of raw maps. - Add unit tests for resolving options and testing PathDoesNotExistRule with custom exclusions.
![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 introduces support for rule-specific custom options in dart_skills_lint.yaml and refactors the path-does-not-exist check into a class-based, configurable SkillRule supporting an exclude option. The feedback highlights critical improvements, including preventing rules from being silently disabled when the severity key is omitted in map-based configurations, avoiding calling overridable getters inside the ConfigurableSkillRule constructor, and validating regular expression patterns during options validation to prevent unhandled crashes.
| if (value is YamlMap) { | ||
| final severityStr = value[_severityKey]?.toString() ?? ''; | ||
| rules[ruleName] = _parseSeverity(severityStr); |
There was a problem hiding this comment.
When parsing map-based rule configurations, if the severity key is omitted, the rule's severity is currently set to disabled (since _parseSeverity('') defaults to disabled). This silently disables the rule when a user only wants to configure custom options. We should only override the severity if the severity key is explicitly present in the map.
| if (value is YamlMap) { | |
| final severityStr = value[_severityKey]?.toString() ?? ''; | |
| rules[ruleName] = _parseSeverity(severityStr); | |
| if (value is YamlMap) { | |
| if (value.containsKey(_severityKey)) { | |
| final severityStr = value[_severityKey]?.toString() ?? ''; | |
| rules[ruleName] = _parseSeverity(severityStr); | |
| } |
| abstract class ConfigurableSkillRule extends SkillRule { | ||
| ConfigurableSkillRule(this.customRuleOptions) { | ||
| _validateOptions(); | ||
| } | ||
|
|
||
| /// The parsed custom configuration options for this rule. | ||
| final CustomRuleOptions? customRuleOptions; | ||
|
|
||
| void _validateOptions() { | ||
| final CustomRuleOptions? opts = customRuleOptions; | ||
| if (opts == null) { | ||
| return; | ||
| } | ||
| // Rules must have a unique name so we can assume one match. | ||
| final Iterable<CheckType> checkMatches = RuleRegistry.allChecks.where((c) => c.name == name); | ||
| if (checkMatches.isEmpty) { | ||
| return; | ||
| } | ||
| final CheckType check = checkMatches.first; | ||
| final List<String> errors = check.validateOptions(opts); | ||
| if (errors.isNotEmpty) { | ||
| throw ArgumentError(errors.join('\n')); | ||
| } | ||
| } |
There was a problem hiding this comment.
Calling overridable members (like the name getter) inside a constructor is an anti-pattern in Dart because the subclass is not yet fully initialized. If a subclass overrides name as a field, it will be null during super constructor execution. We should pass the rule name explicitly to the ConfigurableSkillRule constructor to ensure safe initialization.
abstract class ConfigurableSkillRule extends SkillRule {
ConfigurableSkillRule(this.customRuleOptions, String ruleName) {
_validateOptions(ruleName);
}
/// The parsed custom configuration options for this rule.
final CustomRuleOptions? customRuleOptions;
void _validateOptions(String ruleName) {
final CustomRuleOptions? opts = customRuleOptions;
if (opts == null) {
return;
}
// Rules must have a unique name so we can assume one match.
final Iterable<CheckType> checkMatches = RuleRegistry.allChecks.where((c) => c.name == ruleName);
if (checkMatches.isEmpty) {
return;
}
final CheckType check = checkMatches.first;
final List<String> errors = check.validateOptions(opts);
if (errors.isNotEmpty) {
throw ArgumentError(errors.join('\n'));
}
}| class PathDoesNotExistRule extends ConfigurableSkillRule { | ||
| PathDoesNotExistRule({required this.severity, CustomRuleOptions? customRuleOptions}) | ||
| : super(customRuleOptions) { |
There was a problem hiding this comment.
Pass the ruleName static constant to the super constructor to avoid calling the overridable name getter during object construction.
| class PathDoesNotExistRule extends ConfigurableSkillRule { | |
| PathDoesNotExistRule({required this.severity, CustomRuleOptions? customRuleOptions}) | |
| : super(customRuleOptions) { | |
| class PathDoesNotExistRule extends ConfigurableSkillRule { | |
| PathDoesNotExistRule({required this.severity, CustomRuleOptions? customRuleOptions}) | |
| : super(customRuleOptions, ruleName) { |
| } | ||
| final Type expectedType = allowedOptions[key]!; | ||
| final Object? actualValue = options[key]; | ||
| if (actualValue != null && !_isTypeValid(actualValue, expectedType)) { | ||
| errors.add( | ||
| 'Invalid type for option "$key" in rule "$name". ' |
There was a problem hiding this comment.
An invalid regular expression pattern in the exclude option of path-does-not-exist will cause the linter to crash with an unhandled FormatException during rule construction. We should validate that the pattern is a valid regular expression during options validation so that configuration errors are reported cleanly to the user.
if (actualValue != null && !_isTypeValid(actualValue, expectedType)) {
errors.add(
'Invalid type for option "$key" in rule "$name". '
'Expected $expectedType, got "$actualValue" (${actualValue.runtimeType}).',
);
} else if (actualValue is String && name == 'path-does-not-exist' && key == 'exclude') {
try {
RegExp(actualValue);
} on FormatException catch (e) {
errors.add(
'Invalid regular expression for option "$key" in rule "$name": ${e.message}',
);
}
}
reidbaker-agent
changed the title
… to optionsSchema - Add type-safe getters (getString, getInt, getBool, getStringList) to CustomRuleOptions. - Update PathDoesNotExistRule to retrieve options using the new type-safe getters. - Rename allowedOptions to optionsSchema inside CheckType to distinguish the schema definition from configuration values. - Clean up all references across EntryPoint, Registry, and tests to match optionsSchema.
2 participants
feat: Support Configurable Rule-Specific Options and Custom Exclusions
Context & Goal
Previously, rule evaluations were static and could not accept custom runtime configurations or parameters from the configuration file (
dart_skills_lint.yaml) or command-line overrides.This PR introduces the concept of rule-specific options/parameters to make rules configurable. We then leverage this feature to add a custom
excludeoption topath-does-not-exist, allowing users to skip validation on matching skill folders (e.g. workspace or integration test subdirectories).Architectural Design & Key Features
CustomRuleOptionsas a type-safe wrapper for rule-specific parameters, replacing loose maps throughout the validation pipeline.ConfigurableSkillRuleas the base class for any rule that accepts custom options.path-does-not-exist:PathDoesNotExistRuleto extendConfigurableSkillRule.excludeoption (regular expression pattern string) to skip directories that match the pattern.CheckType.validateOptions.RuleRegistry.allChecks.ArgumentError).ValidationSessionresolves target-specific, global, and CLI flag overrides into the final options map.