Extract rule: template-require-form-method#2603
Extract rule: template-require-form-method#2603NullVoxPopuli wants to merge 1 commit intoember-cli:masterfrom
Conversation
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
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
Split from #2371.