fix(rubocop): declare the cop options config/default.yml only documented - #9
Merged
Merged
Conversation
## Summary
RuboCop builds each cop's supported-parameter list from the keys actually
present in `config/default.yml` (`ConfigValidator#each_invalid_parameter`
checks `default_config.key?(param)`). Four working options were invisible to
that check, so every run warned they were unsupported:
- `Glyphs/IconResolution` `Libraries` — comment only
- `Glyphs/LegacyIconHelper` `Mappings` — comment only
- `Glyphs/LegacyIconHelper` `LibraryComponents` — absent entirely
- `Glyphs/PreferLibraryComponent` `LibraryComponents` — absent entirely
The last two are the sharpest case: `LegacyIconHelper`'s own `MSG_UNKNOWN`
tells users to "add a `LibraryComponents` mapping", and doing so warned.
Each is now declared with an empty default. `DEFAULT_LIBRARIES.merge({})` and
`LIBRARY_TO_COMPONENT.merge({})` are no-ops, and `LegacyIconHelper#mappings`
already falls back to `DEFAULT_MAPPINGS` when the configured hash is empty, so
behaviour is unchanged for everyone.
## Test Coverage
- plugin_spec: derives every `cop_config["Key"]` read by each registered Glyphs
cop (and its `RuboCop::Cop::Glyphs::*` mixins) from source and asserts
`config/default.yml` declares it — a new undeclared option now fails the suite
- icon_resolution_spec: `Libraries: {}` still resolves via the built-in defaults
- legacy_icon_helper_spec: `Mappings: {}` / `LibraryComponents: {}` still apply
the built-in helper map and library components
- prefer_library_component_spec: `LibraryComponents: {}` still applies defaults
## Verification
- [x] bundle exec rubocop lib spec passes
- [x] bundle exec rspec passes (137 examples)
- [x] Real `rubocop` run against a project config setting all four options:
4 warnings before, 0 after, byte-identical offences
Refs #7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7.
The problem
RuboCop derives each cop's supported-parameter list from the keys actually present in the gem's
config/default.yml—ConfigValidator#each_invalid_parameterdoes a literaldefault_config.key?(param). An option that a cop reads but the file only documents in a comment is invisible to that check and gets reported as unsupported on every single run.Issue #7 reports one instance. Reproducing it against a real
rubocopinvocation surfaced four:config/default.ymlGlyphs/IconResolutionLibrariesGlyphs/LegacyIconHelperMappingsGlyphs/LegacyIconHelperLibraryComponentsGlyphs/PreferLibraryComponentLibraryComponentsThe bottom two are the sharpest case:
LegacyIconHelper's ownMSG_UNKNOWNoffence message tells the user to "add aLibraryComponentsmapping for library X" — and following that advice produced a warning telling them the option does not exist.All four options genuinely work, which is what makes the warning harmful: the natural reaction is to delete working configuration.
The fix
Declare each key with an empty default, keeping the example as a comment above it. The reads are all no-ops against an empty hash:
DEFAULT_LIBRARIES.merge({})andLIBRARY_TO_COMPONENT.merge({})return the built-in maps unchanged.LegacyIconHelper#mappingsalready special-cases empty (configured.empty? ? DEFAULT_MAPPINGS : ...), soMappings: {}keeps the built-in helper map.Nobody's behaviour changes; RuboCop just stops lying about the option.
Test plan
spec/rubocop/plugin_spec.rbgains a drift guard: it scans each registered Glyphs cop — and everyRuboCop::Cop::Glyphs::*module the cop includes, which is whereLibraryComponentsactually lives — forcop_config["Key"], then assertsconfig/default.ymldeclares each one. Verified it fails correctly by deletingLibraries: {}and watching it reportGlyphs/IconResolution reads undeclared Libraries.{}as "use built-ins" is caught.End-to-end before/after
Same project config (all four options set), same target file, only
config/default.ymldiffering:The identical offence list is the important half: it proves the declarations did not quietly change how the options resolve.
Suite
bundle exec rubocop lib spec— cleanbundle exec rspec— 137 examples, 0 failuresDeviations & judgment calls
Scope widened from one warning to four. The issue names only
Libraries. The reproduction showed the identical declaration gap on three more options across the other two cops. Fixing only the reported one would have left the exact same bug in place for the next person, so all four are fixed together — one root cause, one commit.The regression guard derives the parameter list from source rather than hardcoding it. A spec listing the expected keys would only restate
config/default.ymland would not catch the next option added to a cop without a declaration. Scanning the cop classes plus theirRuboCop::Cop::Glyphs::*mixins forcop_config["Key"]mirrors RuboCop's own check and turns this class of bug into a test failure. Cost: the guard is regex-over-source, so a dynamically-built key (cop_config[some_var]) would slip past it — no such call exists today.Out-of-scope discovery, not fixed here:
require "glyphs/rubocop"raisesNameError.lib/rubocop/cop/glyphs/library_call_helpers.rb:15references::Glyphs::IconReference, butlib/glyphs/rubocop.rbnever requiresglyphs. It works today only because theplugins:path loads the gem first — but README line 192 documentsrequire: [glyphs/rubocop]as a supported alternative, and that path is broken. Different bug, different symptom; it deserves its own issue rather than being smuggled in here.Not touched: Glyphs/IconResolution silently skips validation when the icon directory is missing (fails open) #8 (fail-open on a missing icons directory). Referenced by Glyphs/IconResolution:
Librariesis documented in config/default.yml but not declared, so RuboCop warns it is unsupported #7 as a companion, but it is a behaviour change, not a declaration fix.README gained a
LibraryComponentsexample for both cops that read it. It was previously undocumented despite being named in a cop offence message — arguably the reason the declaration was missed in the first place.