From 8598c25188049ce68b26b88b3bc19dda7c7065e1 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Sat, 25 Jul 2026 08:57:53 +0200 Subject: [PATCH] fix(rubocop): report a missing icon directory instead of failing open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- CHANGELOG.md | 15 +++++ README.md | 15 +++++ config/default.yml | 3 + lib/rubocop/cop/glyphs/icon_resolution.rb | 60 +++++++++++++++++-- .../fixtures/svg/icons/emptylib/regular/.keep | 0 .../cop/glyphs/icon_resolution_spec.rb | 47 ++++++++++++++- 6 files changed, 131 insertions(+), 9 deletions(-) create mode 100644 spec/fixtures/svg/icons/emptylib/regular/.keep diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d41152..a384043 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ ### Fixed +- **`Glyphs/IconResolution` no longer fails open on a missing icon directory.** + A directory that did not exist produced the same empty list as one that + existed with no SVGs, and both were read as "nothing to check" — so a library + that was never synced, or a `DefaultVariant` naming a variant the project does + not have, silently disabled the cop for every call site of that library. Green + cop, green CI, no validation at all, indefinitely. `load_icons` now returns + `nil` for an absent directory and `[]` for an empty one; the absent case warns + once per library/variant, naming the path. The synced-but-empty case still + passes silently. Refs #8 + - **Cop options are declared, so RuboCop stops calling them unsupported.** `config/default.yml` documented `Libraries` (`Glyphs/IconResolution`) and `Mappings` (`Glyphs/LegacyIconHelper`) only in comments, and never mentioned @@ -24,6 +34,11 @@ ### Added +- **`Glyphs/IconResolution` gained a `Strict` option** (default `false`). It + escalates the missing-icon-directory warning above to an offence, so projects + that depend on this cop can fail closed in CI instead of trusting a warning + not to scroll past. + - **Dynamic icon calls are now resolved from source.** `SourceScanner` no longer silently skips `LucideIcon(some_var)` / `PhosphorIcon(tile[:icon])` — it harvests the literal name from two places so the pruner keeps it: diff --git a/README.md b/README.md index 07dd2a1..d569e01 100644 --- a/README.md +++ b/README.md @@ -235,8 +235,23 @@ Glyphs/IconResolution: # Libraries merges over the built-in defaults (component => Dir/DefaultVariant): Libraries: PhosphorIcon: { Dir: phosphor, DefaultVariant: regular } + # Escalate a missing icon directory from a warning to an offence: + Strict: false ``` +If the directory a library resolves to does not exist — the library was never synced, or +`DefaultVariant` names a variant you do not have — the cop cannot validate any of that +library's call sites. Rather than pass silently, it warns once per library/variant: + +``` +[Glyphs/IconResolution] Icon directory `app/assets/svg/icons/phosphor/regular` not found, +so `PhosphorIcon` names are not validated. Sync the library or fix `IconsPath`/`Libraries`. +``` + +Set `Strict: true` to make that an offence instead, so CI fails closed rather than reporting +green while checking nothing. A directory that exists but ships no SVGs is not a +misconfiguration and stays silent either way. + ### Glyphs/PreferLibraryComponent ```ruby diff --git a/config/default.yml b/config/default.yml index ef73c4c..b1744dd 100644 --- a/config/default.yml +++ b/config/default.yml @@ -20,6 +20,9 @@ Glyphs/IconResolution: # Merges over the built-in component => directory/variant defaults, e.g. # PhosphorIcon: { Dir: phosphor, DefaultVariant: light } Libraries: {} + # An icon directory that does not exist warns once per library/variant. + # Strict turns that warning into an offence so CI fails closed. + Strict: false Glyphs/PreferLibraryComponent: Description: 'Prefer library-specific components over generic Icon(..., library: ...).' diff --git a/lib/rubocop/cop/glyphs/icon_resolution.rb b/lib/rubocop/cop/glyphs/icon_resolution.rb index 78ed68a..f8abe4e 100644 --- a/lib/rubocop/cop/glyphs/icon_resolution.rb +++ b/lib/rubocop/cop/glyphs/icon_resolution.rb @@ -11,6 +11,11 @@ module Glyphs # calls (`_lucide(:house)`) are validated. A literal `variant:` keyword is # honored; calls with dynamic names or variants are skipped. # + # A directory that does not exist is a configuration error, not "nothing + # to check": it warns once per library/variant, or — with `Strict` — adds + # an offence, so the cop can never quietly validate nothing. A directory + # that exists but ships no SVGs stays silent. + # # @example # # bad # LucideIcon(:non_existent_icon) @@ -51,10 +56,14 @@ class IconResolution < Base ICONIFY_PATTERN = /\biconify\s+(lucide|phosphor|heroicons)--([a-z0-9-]+)/ + DEFAULT_ICONS_PATH = "app/assets/svg/icons" + RAW_MSG = "Use `%{component}(:%{symbol}, class: \"%{rest}\")` instead of raw `iconify %{library}--…` class." RAW_DSTR_MSG = "Use `LucideIcon(name, class: ...)` etc. instead of building a raw " \ "`iconify --…` class string." MISSING_MSG = "Icon `%{name}` not found in %{library}/%{variant}. %{suggestion}" + MISSING_DIR_MSG = "Icon directory `%{path}` not found, so `%{component}` names are not validated. " \ + "Sync the library or fix `IconsPath`/`Libraries`." def on_str(node) return if node.parent&.dstr_type? @@ -97,7 +106,7 @@ def libraries @libraries ||= DEFAULT_LIBRARIES.merge(cop_config["Libraries"] || {}) end - def check_call(node, _component, library) + def check_call(node, component, library) name = literal_name(node.first_argument) return unless name @@ -105,6 +114,7 @@ def check_call(node, _component, library) return if variant == :dynamic available = available_icons(library["Dir"], variant) + return report_missing_directory(node, component, library["Dir"], variant) if available.nil? return if available.empty? return if available.include?(name) @@ -124,6 +134,21 @@ def check_call(node, _component, library) end end + # The directory the cop was told to read is missing, so every call for + # this library goes unchecked. Report it rather than pass silently. + def report_missing_directory(node, component, library_dir, variant) + message = format(MISSING_DIR_MSG, path: configured_directory(library_dir, variant), component:) + return add_offense(node.first_argument, message:) if cop_config["Strict"] + + self.class.warn_once("[Glyphs/IconResolution] #{message}") + end + + # Built from the unexpanded `IconsPath` so the message shows the path as + # the project wrote it, not an absolute machine-specific one. + def configured_directory(library_dir, variant) + self.class.directory_for(cop_config["IconsPath"] || DEFAULT_ICONS_PATH, library_dir, variant) + end + def variant_for(node, library) pair = variant_pair(node) return library["DefaultVariant"] if pair.nil? @@ -258,7 +283,7 @@ def damerau_levenshtein(left, right) end def icons_base_path - @icons_base_path ||= File.expand_path(cop_config["IconsPath"] || "app/assets/svg/icons", Dir.pwd) + @icons_base_path ||= File.expand_path(cop_config["IconsPath"] || DEFAULT_ICONS_PATH, Dir.pwd) end def available_icons(library_dir, variant) @@ -266,16 +291,39 @@ def available_icons(library_dir, variant) end class << self + # nil when the directory is absent, [] when it exists but ships no + # SVGs — the caller has to tell those apart. Memoized with `key?` (not + # `||=`) so a nil result is not re-probed on every call site. def available_icons_for(base_path, library_dir, variant) @available_icons_cache ||= {} - @available_icons_cache[[base_path, library_dir, variant]] ||= load_icons(base_path, library_dir, variant) + key = [base_path, library_dir, variant] + return @available_icons_cache[key] if @available_icons_cache.key?(key) + + @available_icons_cache[key] = load_icons(directory_for(base_path, library_dir, variant)) + end + + def directory_for(base_path, library_dir, variant) + File.join(*[base_path, library_dir, variant].compact.reject { |part| part.to_s.empty? }) + end + + # RuboCop builds a fresh cop instance per file, so deduplicating the + # missing-directory warning has to outlive the instance. + def warn_once(message) + @warned_messages ||= {} + return if @warned_messages.key?(message) + + @warned_messages[message] = true + warn message + end + + def reset_warnings! + @warned_messages = nil end private - def load_icons(base_path, library_dir, variant) - path = File.join(*[base_path, library_dir, variant].compact.reject { |part| part.to_s.empty? }) - return [] unless Dir.exist?(path) + def load_icons(path) + return nil unless Dir.exist?(path) Dir.children(path).filter_map { |file| File.basename(file, ".svg") if file.end_with?(".svg") }.sort end diff --git a/spec/fixtures/svg/icons/emptylib/regular/.keep b/spec/fixtures/svg/icons/emptylib/regular/.keep new file mode 100644 index 0000000..e69de29 diff --git a/spec/rubocop/cop/glyphs/icon_resolution_spec.rb b/spec/rubocop/cop/glyphs/icon_resolution_spec.rb index 5814a52..366c189 100644 --- a/spec/rubocop/cop/glyphs/icon_resolution_spec.rb +++ b/spec/rubocop/cop/glyphs/icon_resolution_spec.rb @@ -4,6 +4,10 @@ let(:config) { RuboCop::Config.new("Glyphs/IconResolution" => cop_config) } let(:cop_config) { { "IconsPath" => "spec/fixtures/svg/icons" } } + # The missing-directory warning is deduplicated for the whole process (RuboCop + # builds a fresh cop instance per file), so examples must not inherit it. + before { described_class.reset_warnings! } + context "with component calls" do it "accepts icons that exist" do expect_no_offenses(<<~RUBY) @@ -127,11 +131,48 @@ end end - context "when the icons path does not exist" do + context "when the icons directory does not exist" do let(:cop_config) { { "IconsPath" => "spec/fixtures/nope" } } - it "reports nothing" do - expect_no_offenses("LucideIcon(:anything_at_all)") + it "warns instead of silently validating nothing" do + expect { expect_no_offenses("LucideIcon(:anything_at_all)") } + .to output(%r{\[Glyphs/IconResolution\].*spec/fixtures/nope/lucide/outline}).to_stderr + end + + it "warns once per library and variant, not once per call site" do + expect { expect_no_offenses("LucideIcon(:one)\nLucideIcon(:two)") }.to output.to_stderr + expect { expect_no_offenses("LucideIcon(:three)") }.not_to output.to_stderr + expect { expect_no_offenses("HeroIcon(:four)") }.to output(%r{nope/heroicons/outline}).to_stderr + end + end + + context "when the icons directory does not exist and Strict is enabled" do + let(:cop_config) { { "IconsPath" => "spec/fixtures/nope", "Strict" => true } } + + it "reports an offence at the call site instead of warning" do + expect do + expect_offense(<<~RUBY) + LucideIcon(:anything_at_all) + ^^^^^^^^^^^^^^^^ Icon directory `spec/fixtures/nope/lucide/outline` not found, so `LucideIcon` names are not validated. Sync the library or fix `IconsPath`/`Libraries`. + RUBY + end.not_to output.to_stderr + + expect_no_corrections + end + end + + # Load-bearing: a library that is synced but genuinely ships no SVGs is not a + # misconfiguration, and must not produce an offence on every call site. + context "when the icons directory exists but holds no SVGs" do + let(:cop_config) do + { + "IconsPath" => "spec/fixtures/svg/icons", + "Libraries" => { "EmptyIcon" => { "Dir" => "emptylib", "DefaultVariant" => "regular" } } + } + end + + it "stays silent" do + expect { expect_no_offenses("EmptyIcon(:whatever_at_all)") }.not_to output.to_stderr end end