fix(rubocop): report a missing icon directory instead of failing open - #10
Merged
Merged
Conversation
## Summary
`Glyphs/IconResolution` collapsed "directory absent" and "directory exists but
holds no SVGs" into the same empty array, and read both as "nothing to check".
A library that was never synced — or a `DefaultVariant` naming a variant the
project does not have — therefore disabled the cop for every call site of that
library, with no offence, no warning, and no output of any kind. Green cop,
green CI, guarantee silently absent.
`load_icons` now returns `nil` for an absent directory and `[]` for an empty
one. `check_call` reports the absent case (naming the path) and keeps skipping
the empty one, which is load-bearing: a synced-but-empty library is not a
misconfiguration.
Reporting is a warning by default, deduplicated per library/variant for the
process — RuboCop mobilizes a fresh cop instance per file, so the dedup lives on
the class. The new `Strict` option escalates it to an offence so projects that
depend on this cop can fail closed in CI.
## Test Coverage
- warns naming the missing path instead of silently validating nothing
- warns once per library/variant, not once per call site, across separate runs
- Strict: adds an offence at the call site, emits no warning, no autocorrection
- exists-but-empty directory stays silent (new spec/fixtures/svg/icons/emptylib)
- every existing resolution/suggestion/autocorrect spec unchanged
## Verification
- [x] bundle exec rubocop lib spec passes
- [x] bundle exec rspec passes (140 examples)
- [x] Reproduced the issue's scenario: before, the bogus Phosphor name passed;
after, the missing phosphor/regular directory is named, with the Lucide
and Heroicons offences byte-identical
- [x] Real two-file run: 3 Phosphor call sites produce exactly 1 warning
Refs #8
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 #8.
The problem
load_iconsreturned[]both when the icon directory was absent and when it existed with no SVGs, andcheck_callread[]as "nothing to validate". So a library that was never synced — or aDefaultVariantnaming a variant the project does not have — silently disabled the cop for every call site of that library. No offence, no warning, no output at all.That is the worst shape a bug can take in a linter: the cop is green, CI is green, and the guarantee it exists to provide is simply gone. Nothing ever draws attention to it.
The
[]guard is load-bearing, though — a library that ships no icons must not produce an offence on every call. So the fix is not to delete the guard, it is to stop conflating two different states.The fix
load_iconsreturnsnilfor an absent directory and[]for an empty one.check_callthen branches:nil— directory absentStrict)[]— synced, no SVGsReporting is a warning by default and an offence under the new
Strictoption, per the issue's suggestion. Offence-by-default would be louder but is a breaking change: every project that references a library it does not sync would start failing CI on upgrade.Strictlets projects that depend on this cop opt into failing closed.Test plan
Strict: trueadds an offence at the call site, emits no warning, and offers no autocorrection.spec/fixtures/svg/icons/emptylib/fixture pins the load-bearing guard.End-to-end, using the issue's own reproduction
A project syncing only Phosphor
light, sophosphor/regular(the built-inDefaultVariant) is absent:Also verified on a real two-file run: 3 Phosphor call sites across 2 files produce exactly 1 warning.
Suite
bundle exec rubocop lib spec— cleanbundle exec rspec— 140 examples, 0 failuresDeviations & judgment calls
Warn dedup had to live on the class, not the instance. RuboCop's
Runnermobilizes a freshTeam— and therefore fresh cop instances — per file, so instance state would warn once per file rather than once per run. This is the same reason the existing icons cache is class-level. Addedreset_warnings!so specs are order-independent; precedent is the gem's existingGlyphs.reset_cache!.The memo moved from
||=tokey?.@available_icons_cache[key] ||= ...never memoizes anil, so an absent directory would re-probe the filesystem at every call site. That is the scan hot path, so thekey?form keeps it to oneDir.exist?per library/variant.Message paths are built from the unexpanded
IconsPath.icons_base_pathexpands againstDir.pwd, so reusing it would put absolute machine-specific paths into offence messages and make specs unportable. A second one-line builder renders the path as the project wrote it.Omitted
VersionChangedfrom theconfig/default.ymlentry. RuboCop convention is to bump it when a cop gains a parameter, but that means naming the next release number, andrake releaseowns versioning — a feature PR guessing0.3.0would be wrong if the release lands as0.2.4. Happy to add it if you would rather pin it now.Strictoffences anchor on the name argument (the same node as the missing-icon offence) and are deliberately not autocorrectable — there is no safe edit for "this directory does not exist".The fix(rubocop): declare the cop options config/default.yml only documented #9 drift guard earned itself immediately. Adding
cop_config["Strict"]without declaring it inconfig/default.ymlfailedspec/rubocop/plugin_spec.rbbefore I ran anything by hand — the exact class of bug Glyphs/IconResolution:Librariesis documented in config/default.yml but not declared, so RuboCop warns it is unsupported #7 was about, caught automatically one PR later.