Skip to content

Extract rule: template-require-form-method#2603

Open
NullVoxPopuli wants to merge 1 commit intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-require-form-method
Open

Extract rule: template-require-form-method#2603
NullVoxPopuli wants to merge 1 commit intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-require-form-method

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-form-method

General Correctness

1. parseConfig returns false for invalid allowedMethods values, silently disabling the rule. The original throws an error with a descriptive message when invalid config is provided. The PR's approach of returning false (which makes create() return {}, i.e., no-op) means misconfigured rules silently do nothing. Consider throwing or at least logging a warning for invalid configurations, as the original does.

2. The messages object is empty but errors are reported via message string directly. This works but is inconsistent with ESLint best practices. Using messageId with defined messages in meta.messages is preferred. Minor style point.

3. The original reports when value.type !== 'TextNode' by skipping (returning early), which means dynamic values like method={{foo}} are silently allowed. The PR matches this behavior correctly -- dynamic values are not flagged.

4. Missing edge case: method attribute with no value (e.g., <form method></form>). Looking at the original, if typeAttribute exists but has no value or a non-TextNode value, it returns early without error. The PR's code checks methodAttribute.value && methodAttribute.value.type === 'GlimmerTextNode', so a bare method attribute with no value would fall through without reporting. This matches the original's behavior.

5. Tests are thorough. Both gjs and hbs modes are tested. Config options are exercised.

Scope Analysis (gjs/gts)

This rule only checks GlimmerElementNode for node.tag === 'form' -- a plain HTML element tag name. No scope analysis needed since this is a structural/syntactic check on HTML elements, not helper/component name matching.

Summary

Good migration. The main concern is the silent no-op on invalid config vs. the original's error-throwing behavior.

🤖 Automated review comparing with ember-template-lint source

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