Extract rule: template-simple-modifiers#2618
Conversation
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
Review: template-simple-modifiers
Compared against ember-template-lint simple-modifiers.js.
General Correctness
-
Faithful port: The logic correctly mirrors the original ETL rule. The original checks
SubExpressionnodes wherenode.path.original === 'modifier', validates the first param is aStringLiteralorPathExpression, and reports when missing or invalid. The ESLint port does the same with the appropriate Glimmer AST node types. Looks good. -
Error message: Matches the original exactly. Good.
-
meta.messagesis empty but inline messages are used: Themessages: {}object is empty, yetcontext.report()uses inlinemessagestrings. This is inconsistent with other rules in the plugin. Consider defining amessageIdinmessagesand usingmessageIdincontext.report()calls instead of inlinemessage. This would also make tests more robust (testingmessageIdrather than matching full strings). -
Tests: Good coverage for both gjs and hbs modes. The test cases match the original ETL test scenarios well.
-
Missing
recommended: false: The docs metadata hasrecommended: falsebut this is fine -- just noting consistency.
Scope Analysis (gjs/gts)
This rule checks node.path.original === 'modifier' to match the built-in modifier helper. In gjs/gts files, a user could shadow modifier with a local import:
import { modifier } from 'some-custom-lib';
// This 'modifier' is not Ember's built-in modifier helperThe rule should ideally use context.sourceCode.getScope(node) to verify that modifier is not shadowed by a local JS binding before reporting. Without this check, the rule would incorrectly flag custom functions named modifier that happen to be used in sub-expressions. That said, this is an edge case since the modifier keyword is quite specific to Ember's API, so this is a low-severity concern.
🤖 Automated review comparing with ember-template-lint source
1bef554 to
de59887
Compare
Split from #2371.