Extract rule: template-require-lang-attribute#2608
Conversation
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
Review: template-require-lang-attribute
General Correctness
1. The original uses an external country-codes.js helper for validation. The PR embeds a hardcoded set of ISO 639-1 language codes (COMMON_LANG_CODES). This is a reasonable approach but differs from the original's validation logic. The original's hasValidValue splits by - and checks that every part is in the codes list OR the full value is in the list. The PR's isValidLangTag only checks the first part (primary language subtag). This means:
- Original:
"en-US"-- checks both"en"and"us"are in codes. Ifcodesincludes country codes like"us", this passes. - PR:
"en-US"-- only checks"en"is inCOMMON_LANG_CODES. Region subtags are ignored.
The PR's approach is actually more correct for BCP 47 validation, since region subtags (like US, GB) are not language codes and shouldn't be validated against a language code list. The original's approach of checking every part may reject valid tags like "en-US" if "us" isn't in the country codes list.
2. Error messages differ from original. The original uses a single message: "The \` element must have the `lang` attribute with a valid value" for all cases. The PR has three distinct messages (missing, empty, invalid`), which is actually an improvement -- more specific error reporting.
3. The original uses a single error message for both missing and invalid lang. The PR's approach of separate missing, empty, and invalid messages with data interpolation ({{value}}) is better UX.
4. The validateValues option handling matches the original -- when false, the rule still checks for presence and non-emptiness, just not BCP 47 validity. Good.
5. Double reporting bug in original is NOT replicated. The original has a subtle bug where it logs an error for missing lang AND then continues to check if lang has a valid value (even though it was just determined to be missing). The PR correctly returns after reporting missing, avoiding this double-report. Good fix.
6. Tests are comprehensive with both gjs and hbs modes, and cover missing, empty, invalid, valid, and dynamic lang values.
Scope Analysis (gjs/gts)
This rule only checks GlimmerElementNode for node.tag === 'html' -- HTML element inspection. No scope analysis needed.
Summary
Good migration with some improvements over the original (separate error messages, no double-reporting bug, more correct BCP 47 subtag validation). The hardcoded language code set vs. external file is a trade-off but reasonable.
🤖 Automated review comparing with ember-template-lint source
75b6005 to
3a62ce5
Compare
Split from #2371.