Skip to content

Extract rule: template-sort-invocations#2620

Open
NullVoxPopuli wants to merge 3 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-sort-invocations
Open

Extract rule: template-sort-invocations#2620
NullVoxPopuli wants to merge 3 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-sort-invocations

Conversation

@NullVoxPopuli
Copy link
Contributor

Split from #2371.

Copy link

@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-sort-invocations

Compared against ember-template-lint sort-invocations.js.

General Correctness

  1. reported flag limits to one error per file: The rule uses a let reported = false flag that prevents reporting more than one sorting violation per file. The original ETL rule does NOT have this behavior -- it reports all violations found across all nodes. This means if a file has multiple unsorted elements, the ESLint rule will only report the first one. This is a behavioral difference from the original. Was this intentional? If so, it should be documented. If not, the reported guard should be removed.

  2. No autofix: The original ETL rule has full fix mode support (sorting attributes, modifiers, hash pairs, and reordering splattributes). The ESLint port sets fixable: null. This is a significant feature loss. The original marks all violations as isFixable: true. Consider implementing autofix or at least noting this limitation in the docs.

  3. localeCompare vs simple string comparison: The ESLint port uses nameA.localeCompare(nameB) while the original uses simple > / < string comparison. This could produce different sort orders for non-ASCII attribute names. The original's behavior is more predictable (pure lexicographic). This is a minor behavioral difference but could cause inconsistencies between the two tools.

  4. GlimmerStringLiteral field name: In compareModifiers, the port accesses eventA.value for GlimmerStringLiteral nodes, while the original accesses eventA.original for StringLiteral nodes. Verify that value is the correct property on GlimmerStringLiteral nodes in the ember-eslint-parser -- it might be original or value depending on the parser.

  5. canSkipSplattributesLast logic difference: The original checks attributes.at(-1) and modifiers.at(-1) without first checking array lengths. The ESLint port adds extra guards for empty arrays, which is good defensive coding. However, the original also doesn't have the attributes.length === 1 special-case message logic that the port adds. The port reports attributeOrder when ...attributes is the only attribute, vs. splattributesOrder otherwise. This is a nice UX improvement.

  6. Comprehensive test coverage: The tests cover the main scenarios well. Good to see both gjs and hbs tests.

Scope Analysis (gjs/gts)

This rule only checks structural properties: attribute names, attribute ordering, modifier ordering, hash pair ordering. It does not match any helper/component names by string. No scope analysis needed.


🤖 Automated review comparing with ember-template-lint source

NullVoxPopuli and others added 2 commits March 18, 2026 18:47
The rule had a `reported` flag that, once set to true, prevented any
further errors from being reported in the same file. The original
ember-template-lint rule reports all out-of-order invocations, not just
the first one. Remove the flag so all violations are reported.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…errors

Fix one-error-per-file limitation in template-sort-invocations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants