Skip to content

Extract rule: template-require-valid-form-groups#2615

Closed
NullVoxPopuli wants to merge 2 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-require-valid-form-groups
Closed

Extract rule: template-require-valid-form-groups#2615
NullVoxPopuli wants to merge 2 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-require-valid-form-groups

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-require-valid-form-groups (PR #2615)

Comparison with ember-template-lint source

General correctness:

  1. Parent traversal mechanism: The original ETL uses path.parents() to walk up the ancestor chain, checking all ElementNode ancestors. The ESLint version uses node.parent and manually walks up. This depends on whether the Glimmer ESLint parser sets .parent on nodes. If it does, this works equivalently. If not, hasValidGroupingAncestor would always return false, causing false positives.

  2. legend tag handling differs: The original ETL checks ['legend', 'fieldset'].includes(node.tag) — meaning a <legend> tag itself is treated as a valid form group ancestor. The ESLint version does the same in isValidFormGroup. However, <legend> alone doesn't group form controls — it's <fieldset> that does the grouping, with <legend> as its label. Including <legend> as a valid grouping ancestor is technically wrong in both versions, but at least they match.

  3. hasMultipleFormElementsInParentScope relies on node.parent: The function checks node.parent and requires it to be a GlimmerElementNode. If the parent is the template root (e.g., GlimmerTemplate), this returns false, meaning form elements directly under the template root would never be flagged. The original ETL checks path.parent.node.type === 'ElementNode' — same behavior.

  4. Only checks input elements: Both versions only check <input> elements via FORM_ELEMENTS. Other form elements like <select>, <textarea> are not checked. This matches the original.

Scope analysis (gjs/gts):

Not applicable. This rule checks HTML element tags (input, fieldset, legend) and attributes (role, aria-labelledby). No helper/component name matching.

Tests: Test coverage matches the original ETL tests. Both gjs and hbs modes tested.

Summary: Faithful migration. The main concern is whether node.parent is reliably set by the Glimmer ESLint parser — if so, this is correct.

🤖 Automated review comparing with ember-template-lint source

@NullVoxPopuli NullVoxPopuli force-pushed the nvp/template-lint-extract-rule-template-require-valid-form-groups branch from a7fc196 to 0f01f92 Compare March 19, 2026 22:23
@NullVoxPopuli NullVoxPopuli deleted the nvp/template-lint-extract-rule-template-require-valid-form-groups branch March 19, 2026 22:51
@NullVoxPopuli NullVoxPopuli restored the nvp/template-lint-extract-rule-template-require-valid-form-groups branch March 19, 2026 22:51
@NullVoxPopuli NullVoxPopuli deleted the nvp/template-lint-extract-rule-template-require-valid-form-groups branch March 19, 2026 23:03
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