Unused keys: Handles false positives for child nodes#721
Unused keys: Handles false positives for child nodes#721davidwessman wants to merge 3 commits intomainfrom
Conversation
- When a parent node is used as the key, it causes false positives for its children. - Fixes #713
73af62b to
14fab54
Compare
There was a problem hiding this comment.
Pull request overview
Fixes false positives in i18n-tasks unused where using a parent key (e.g. t(:section)) could cause nested child keys under the same root to be reported as unused (Issue #713).
Changes:
- Updates unused-key filtering to treat a key as used when any ancestor key is used.
- Adds an RSpec regression test reproducing the shadowing case from #713.
- Adds an entry to the Unreleased changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/i18n/tasks/unused_keys.rb |
Adjusts unused key selection to account for ancestor/descendant “used” relationships. |
spec/unused_keys_spec.rb |
Adds a regression spec ensuring nested keys aren’t flagged unused when the root key is used. |
CHANGES.md |
Documents the behavior change in the Unreleased section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TestCodebase.in_test_app_dir { ex.call } | ||
| TestCodebase.teardown |
There was a problem hiding this comment.
TestCodebase.teardown won’t run if the example raises (e.g., a failing expectation), which can leave files behind and potentially affect subsequent specs. Wrap the in_test_app_dir { ex.call } call in an ensure (or use an after hook) so teardown is guaranteed to execute.
| TestCodebase.in_test_app_dir { ex.call } | |
| TestCodebase.teardown | |
| begin | |
| TestCodebase.in_test_app_dir { ex.call } | |
| ensure | |
| TestCodebase.teardown | |
| end |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def unused_tree(locale: base_locale, strict: nil) | ||
| used_key_names = used_tree(strict: true).key_names | ||
| used_key_names = used_tree(strict: true).key_names.to_set | ||
| collapse_plural_nodes!(data[locale].select_keys do |key, _node| | ||
| !ignore_key?(key, :unused) && | ||
| (strict || !used_in_expr?(key)) && | ||
| !used_key_names.include?(depluralize_key(key, locale)) | ||
| !key_used?(depluralize_key(key, locale), used_key_names) | ||
| end) |
There was a problem hiding this comment.
key_used? makes any descendant key count as used when an ancestor key is used. This avoids the unused false positive, but it also changes the semantics of unused (it will now hide genuinely-unused descendants whenever a parent/ancestor is referenced) and does not address the underlying used-keys tree collision where inserting a leaf after its children can replace the subtree (see lib/i18n/tasks/data/tree/siblings.rb#set, which removes an existing node on leaf insert). Consider fixing the collision at the tree-building layer (merge leaf occurrences into an existing node instead of replacing it) so used_tree and other features relying on it remain accurate, and keep unused behavior narrowly scoped to that fix if possible.
| unused_key_names = task.unused_keys.key_names(root: true) | ||
| expect(unused_key_names).not_to include("en.section.item.title") | ||
| expect(unused_key_names).not_to include("en.section.item.subtitle") |
There was a problem hiding this comment.
This spec only asserts that nested keys are not reported as unused; with the current implementation that can pass even if used_tree still drops nested keys when a leaf key is encountered later in the scan (because descendants are treated as used via their ancestor). To ensure the original regression is fully covered (and to protect used_tree/find output), consider also asserting that task.used_tree(strict: true).key_names includes the nested keys.
| unused_key_names = task.unused_keys.key_names(root: true) | |
| expect(unused_key_names).not_to include("en.section.item.title") | |
| expect(unused_key_names).not_to include("en.section.item.subtitle") | |
| unused_key_names = task.unused_keys.key_names(root: true) | |
| used_key_names = task.used_tree(strict: true).key_names | |
| expect(unused_key_names).not_to include("en.section.item.title") | |
| expect(unused_key_names).not_to include("en.section.item.subtitle") | |
| expect(used_key_names).to include("en.section.item.title") | |
| expect(used_key_names).to include("en.section.item.subtitle") |
its children.