Skip to content

Extract rule: template-no-redundant-fn#2567

Merged
NullVoxPopuli merged 3 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-no-redundant-fn
Mar 22, 2026
Merged

Extract rule: template-no-redundant-fn#2567
NullVoxPopuli merged 3 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-no-redundant-fn

Conversation

@NullVoxPopuli
Copy link
Copy Markdown
Contributor

Split from #2371.

Copy link
Copy Markdown

@NullVoxPopuli-ai-agent NullVoxPopuli-ai-agent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: template-no-redundant-fn

Compared against the original ember-template-lint rule no-redundant-fn.

What looks good

  • Core detection logic faithfully matches the original: checks for fn helper with exactly 1 param and no hash pairs.
  • Covers both GlimmerSubExpression (e.g., (fn this.handleClick)) and GlimmerMustacheStatement (e.g., {{fn this.handleClick}}), matching the original's SubExpression and MustacheStatement visitors.
  • The error message includes a helpful suggestion showing the direct function reference.
  • Test cases match the original's test suite exactly (same valid and invalid cases).
  • Both .gjs and .hbs parsers are tested.
  • originallyFrom metadata is present.

Items worth attention

  1. Original restricts to PathExpression params only: The original rule checks params[0].type !== 'PathExpression' and returns early if the single param is not a path expression. The ESLint implementation does not enforce this restriction. This means it would flag (fn "some-string") or (fn 42) which the original would not. In practice these are unlikely inputs, but it's a behavioral difference. The paramText fallback to context.sourceCode.getText(param) handles non-path params gracefully.

  2. Original has autofix support: The original rule supports fix mode, replacing (fn this.action) with this.action (for SubExpressions) and {{fn this.action}} with {{this.action}} (for MustacheStatements). The ESLint rule has fixable: null. Given that the fix is straightforward, this could be a nice enhancement in a follow-up.

  3. Error message style difference: The original uses a terse message: `fn` helpers without additional arguments are not allowed. The ESLint version is more verbose but more helpful: "Unnecessary use of (fn) helper. Pass the function directly instead: {{suggestion}}". This is an improvement.

Summary

This is a clean, faithful port. The test coverage matches the original well. The only meaningful behavioral difference is the missing PathExpression type check on params[0], which is very minor in practice. Nice work.

🤖 Automated review comparing with ember-template-lint source

NullVoxPopuli and others added 2 commits March 21, 2026 22:22
When `fn` is imported from a local module in gjs/gts, it shadows
Ember's built-in helper. Use scope analysis to detect this and
avoid false positives.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@NullVoxPopuli NullVoxPopuli force-pushed the nvp/template-lint-extract-rule-template-no-redundant-fn branch from 69f55fb to 9b7e226 Compare March 22, 2026 02:23
@NullVoxPopuli NullVoxPopuli enabled auto-merge March 22, 2026 02:32
@NullVoxPopuli NullVoxPopuli merged commit 8952249 into ember-cli:master Mar 22, 2026
9 checks passed
@NullVoxPopuli NullVoxPopuli deleted the nvp/template-lint-extract-rule-template-no-redundant-fn branch March 22, 2026 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants