Add unused translation key check#1213
Conversation
jamesmengo
force-pushed
the
jm/unused-translation-key
branch
5 times, most recently
from
9bc501e to
da72b57
Compare
| expect(offenses).to.have.length(0); | ||
| }); | ||
|
|
||
| it('ignores Shopify-provided translation key namespaces by default', async () => { |
jamesmengo
force-pushed
the
jm/unused-translation-key
branch
2 times, most recently
from
201bcb2 to
a8b0b2e
Compare
jamesmengo
force-pushed
the
jm/unused-translation-key
branch
2 times, most recently
from
f0f9845 to
830cf17
Compare
|
|
||
| type ResolvedTranslationKey = | ||
| | { type: 'key'; value: string } | ||
| | { type: 'prefix'; value: string } |
There was a problem hiding this comment.
no suffix because if you prepend a dynamic variable we can't determine what 'tree' / namespace to mark as 'used'
jamesmengo
force-pushed
the
jm/unused-translation-key
branch
from
830cf17 to
ddd3df5
Compare
jamesmengo
force-pushed
the
jm/unused-translation-key
branch
from
ddd3df5 to
1af3e30
Compare
| import { visit } from '../visitor'; | ||
|
|
||
| const TRANSLATION_FILTERS = new Set(['t', 'translate']); | ||
| const STATIC_STRING_FILTERS = new Set(['append', 'prepend']); |
There was a problem hiding this comment.
we only statically recover append / prepend. Any other filter before t / translate is treated as dynamic, because filters like replace, remove, downcase, etc. can rewrite the known prefix
| }, | ||
| }), | ||
| 'snippets/product-form.liquid': ` | ||
| {{ 'products.' | append: 'product.' | append: 'add_to_cart' | t }} |
There was a problem hiding this comment.
@karreiro This is pretty nice, but I'm starting to see how this could scale poorly and get pretty complex. I'm wondering if we should scope this down and just have this valid static keys and to be more aggressive with what we can We consider as dynamic translations which turn off this check
| it('does not infer a static prefix from assigned dynamic translation keys', async () => { | ||
| const offenses = await check( | ||
| { | ||
| 'locales/en.default.json': JSON.stringify({ | ||
| actions: { | ||
| add: 'Add', | ||
| }, | ||
| }), | ||
| 'snippets/cart.liquid': ` | ||
| {% assign translation_key = 'actions.' | append: action %} | ||
| {{ translation_key | t }} | ||
| `, | ||
| }, | ||
| [UnusedTranslationKey], | ||
| ); | ||
|
|
||
| expect(offenses).to.have.length(0); | ||
| }); |
There was a problem hiding this comment.
Is it a "as soon as you have a dynamic, we can't report errors" kind of situation?
There was a problem hiding this comment.
Unfortunately, yes.
I did a best effort approach for suporting append and prepend tags (if they're provided static values), but I'm questioning whether it's worth doing so
There was a problem hiding this comment.
No I think it's fine as is. Honestly this is a tough nut. I'm glad there's this there because otherwise we'd get false positives and those are worse than no red lines at all IMO.
2 participants
What are you adding in this PR?
Adds
UnusedTranslationKey, a new Theme Check rule that reports default locale keys that are not statically referenced by the theme.This checks:
locales/*.default.jsonlocales/*.default.schema.jsonI'm only adding this to
allconfig for now.In the language server, diagnostics are reported only for open files, but the check will check the entire theme.
A principle I'm following here is to try to eliminate false positives by defaulting to not reporting any errors when we can't resolve dynamic translation keys.
This has to strike a balance with making this check completely useless if it's always defaulting to silencing errors. Please scrutinize that specific choice here. Might be Worth trading off additional coverage from this check with simplicity.
{{ 'cart.title' | t }}cart.titleas a literal storefront reference.{{ 'cart.title' | translate }}cart.title;translateis handled the same ast."label": "t:sections.header.settings.title.label"sections.header.settings.title.labelfor schema locale files.{{ 'cart.' | append: 'title' | t }}cart.title.{{ 'title' | prepend: 'cart.' | t }}cart.title.{{ 'products.product.' | append: state | t }}products.product.as a safe prefix and protectsproducts.product.*.{{ 'products.product.' | replace: 'product.', '' | append: state | t }}{{ 'cart.items' | t: count: cart.item_count }}cart.items.oneandcart.items.other.{{ translation_key | t }}{% assign key = 'actions.' | append: action %}+{{ key | t }}shopify.sentence.words_connectorin a locale filecustomer_accounts.order.titlein a locale fileUnsupported filters before
t/translateare treated as dynamic rather than trying to preserve a stale prefix. When a storefront reference is dynamic and cannot be resolved to either an exact key or a safe prefix, storefront unused-key reporting is disabled for that theme.Global
ignoreconfig is respected when collecting translation references, so ignored Liquid files do not protect keys or trigger dynamic-key suppression.What did you learn?
Resolving dynamic keys can be a much larger problem than I initially thought 😆. Oh, silly me.
Tophatting
Before you deploy
changesettheme-app-extension.ymlis not applicable here; generated config did not change