Revert "Revert " feat: add support for configuration error severities in analysis_options.yaml""

[rrousselGit]

Reverts #375

Cf #326

[docs-page]

To view this pull requests documentation preview, visit the following URL:

docs.page/invertase/dart_custom_lint~376

Documentation is deployed and generated using docs.page.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Review	Updated (UTC)
custom-lint-website	Error		Dec 30, 2025 7:07pm

[tsavo-at-pieces]

Code Quality Review

Changes

This is a revert-of-a-revert, effectively re-landing the original PR #326: "feat: add support for configuration error severities in analysis_options.yaml". The feature allows users to override lint rule severities (error/warning/info/ignore) via analysis_options.yaml under a custom_lint.errors key. Changes span 7 files across custom_lint, custom_lint_builder, and custom_lint_core.

Quality Assessment

Architecture & Design (strong):

• The CustomLintConfigs.errors map (Map<String, ErrorSeverity>) is a clean addition that follows the existing patterns for rules.
• Severity override is applied at the right layer -- in _ClientAnalyzerPlugin after convertAnalysisErrors(), with filtering for ignore (NONE) happening via .whereNot() before the severity override .map().
• The analysis_options.yaml merge behavior correctly inherits from include: configs with local overrides taking precedence, matching the existing rules merge pattern.
• The copyWith extension on AnalysisError is well-structured, preserving all fields and only overriding what is needed.

Code quality observations:

1. Missing trailing newline in packages/custom_lint/example/analysis_options.yaml -- the diff shows \ No newline at end of file was introduced. This is a minor formatting issue but should be fixed.

2. Switch expression for severity parsing is clean and exhaustive with a helpful error for unknown values:

   'info' => ErrorSeverity.INFO,
   'warning' => ErrorSeverity.WARNING,
   'error' => ErrorSeverity.ERROR,
   'ignore' => ErrorSeverity.NONE,
   _ => throw UnsupportedError(...)

   One consideration: the matching is case-sensitive. Users writing Error or WARNING will get an UnsupportedError. This could be intentional (strict YAML convention) but is worth noting.

3. Equality and hashCode are correctly updated to include the new errors field, using MapEquality<String, ErrorSeverity>.

4. Test coverage is thorough:

   • Parsing all 4 severity levels
   • Invalid severity handling
   • Merge behavior from included configs
   • Immutability of the errors map
   • CLI integration tests verifying actual output format for error/warning/info/ignore

5. The convertErrorSeverity call on line ~1014 creates a new CustomAnalyzerConverter() instance inside the .map() loop. This is not a performance issue (it is a lightweight object) but could be extracted to reuse the existing converter from 3 lines above.

6. Blank line removal in configs_test.dart (the workspace test) looks like an incidental formatting change -- harmless but unrelated to the feature.

Verdict

APPROVE -- This is a well-implemented feature with solid test coverage. The re-landing via revert-of-revert preserves the original commit cleanly. The only minor nits are the missing trailing newline in the example analysis_options.yaml and the case-sensitivity of severity string matching (which is a design choice, not a bug). Ready to merge after confirming CI passes.

[tsavo-at-pieces]

Fork Maintainer Review (dart_custom_lint)

Reviewing on behalf of open-runtime/dart_custom_lint fork.

Summary

This is a "revert of a revert" by @rrousselGit (Remi Rousselet, the primary maintainer). The history:

1. PR feat: add support for configuration error severities in analysis_options.yaml #326 - Original feature: "add support for configuration error severities" -- merged Dec 30, 2025
2. PR Revert " feat: add support for configuration error severities in analysis_options.yaml" #375 - Revert of feat: add support for configuration error severities in analysis_options.yaml #326 -- merged Dec 30, 2025 (3 minutes after feat: add support for configuration error severities in analysis_options.yaml #326)
3. PR Revert "Revert " feat: add support for configuration error severities in analysis_options.yaml"" #376 - Revert of the revert (this PR) -- re-applies the feature, still open

The feature adds an errors map to CustomLintConfigs that allows users to override lint severity levels in analysis_options.yaml:

custom_lint:
  errors:
    my_lint_rule: error    # override to error
    another_rule: ignore   # suppress entirely

Supported severity levels: error, warning, info, ignore.

Code Changes (322 additions, 8 deletions)

• configs.dart: New errors field on CustomLintConfigs, parsing logic for errors: section with merge from included configs, equality/hashCode updates
• client.dart: Filters out ignore lints, applies severity overrides using a new copyWith extension on AnalysisError
• configs_test.dart: Tests for parsing, immutability, unknown severity handling, and config merging
• analysis_options_test.dart: New integration test file with 4 tests (error, warning, info, ignore severities)
• create_project.dart: Adds analysisOptions parameter to test helper
• README.md: Documents the new feature

Assessment

Well-designed feature with good test coverage. This was authored and is being re-applied by the project maintainer (rrousselGit). Key observations:

• Clean implementation using Dart 3 pattern matching (case final ErrorSeverity errorSeverity)
• Proper immutability with UnmodifiableMapView
• Config merging from included analysis_options files works correctly (included config is base, local overrides win)
• ErrorSeverity.NONE maps to ignore -- clean approach for suppressing lints
• The copyWith extension on AnalysisError is a necessary addition since the analyzer_plugin class doesn't provide one
• One minor nit: the analysis_options.yaml example file loses its trailing newline (cosmetic only)
• Tests are thorough: parsing, immutability, unknown values, merging, and full CLI integration tests

The quick revert-then-revert-of-revert pattern suggests the initial merge may have been premature, but Remi clearly intends to land this feature.

Recommendation

SYNC_FROM_UPSTREAM (once merged) -- This is a significant feature addition by the maintainer with proper test coverage. We want this in our fork for configuration flexibility. Wait for upstream to merge and stabilize before syncing.

[tsavo-at-pieces]

Compatibility Review (Monorepo Impact)

Reviewer context: We maintain the open-runtime/dart_custom_lint fork, consumed as a workspace member in our Dart monorepo (aot_monorepo). All custom_lint* packages (including custom_lint_core and custom_lint_builder) are workspace members with resolution: workspace.

Monorepo Impact

LOW RISK, BENEFICIAL. This PR re-introduces the error severities configuration feature (originally PR #326, reverted in #375, now un-reverted here). The changes touch 3 runtime packages and add tests:

1. custom_lint_core/lib/src/configs.dart -- Adds errors field to CustomLintConfigs with a new Map<String, ErrorSeverity> property. This is an additive API change. Existing code that constructs CustomLintConfigs will need to pass errors: {} or use CustomLintConfigs.empty. Since we do not construct CustomLintConfigs directly in our monorepo (we rely on CustomLintConfigs.parse()), this is safe for us.

2. custom_lint_builder/lib/src/client.dart -- Adds severity override and ignore filtering logic in the analysis error notification pipeline. Adds a copyWith extension on AnalysisError. This is internal to the plugin pipeline and does not affect our monorepo code.

3. custom_lint_core/test/configs_test.dart and custom_lint/test/analysis_options_test.dart -- New test coverage. Our fork would benefit from these tests.

The feature allows analysis_options.yaml to override lint severities:

custom_lint:
  errors:
    my_rule: error  # or warning, info, ignore

This is purely opt-in. Without the errors: key in analysis_options.yaml, behavior is identical to current. The errors map defaults to {} in CustomLintConfigs.empty.

Dependency Concerns

• Adds import 'package:analyzer/error/error.dart' to configs.dart -- but analyzer ^9.0.0 is already a dependency of custom_lint_core, so no new dependency introduced.
• The ErrorSeverity type from analyzer is used in the public API of CustomLintConfigs. This means the analyzer version constraint becomes more tightly coupled to the configs API, but since we already pin analyzer: ^9.0.0, this is not a problem.
• One minor concern: the analysis_options.yaml in packages/custom_lint/example/ loses its trailing newline. This is cosmetic but could cause diff noise.

Recommendation

MERGE -- This is a useful feature that is fully backward-compatible (opt-in via analysis_options.yaml). It was authored by the maintainer (rrousselGit) and was only reverted temporarily. The implementation is clean with good test coverage. No breaking changes to our monorepo workspace setup. We would sync this into our fork after merge.