Add template-lint-disable pragma for template regions#2627
Add template-lint-disable pragma for template regions#2627NullVoxPopuli-ai-agent wants to merge 19 commits intoember-cli:masterfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
NullVoxPopuli
left a comment
NullVoxPopuli
left a comment
There was a problem hiding this comment.
this will need a perf test / benchmark
There was a problem hiding this comment.
Claude Review of template-lint-disable processor
Claude reviewed the final state across all 9 commits. The core approach — wrapping the noop processor to filter messages in postprocess — is sound. A few issues to address:
Must fix
1. Add early-exit in preprocess for files without directives
Every file pays the cost of text.split('\n') + regex matching even when no template-lint-disable comment exists. Possible fix:
preprocess(text, fileName) {
if (!text.includes('template-lint-disable')) {
fileDisableDirectives.delete(fileName);
return [text];
}
// ... existing parsing logic
}This makes the overhead near-zero for the common case (no disable comments).
2. commentLine suppression is over-broad
if (message.line !== directive.commentLine && message.line !== directive.nextLine) {This suppresses errors on the comment line itself, not just the next line. That means:
Would suppress the no-bare-strings error for "Hello world" — the error comes before the comment but is on the same line. The comment explains this is for a TextNode edge case where the newline after the comment belongs to the next TextNode. But it's a blunt instrument.
Options:
- (a) Accept this and document it (ESLint's own
eslint-disable-linealso suppresses the whole line) - (b) Only suppress
commentLinewhenmessage.column > commentEndColumn(would need to track comment position)
I'd go with (a) and document it, but it should be a conscious choice.
3. Misleading test name in gjs tests
it('supports template-lint rule name format (maps to ember/ prefix)', async () => {
// ...
{{! template-lint-disable no-undef }}This test passes because no-undef matches directly as an ESLint rule ID (ruleId === disableRuleName), not because of the ember/template- prefix mapping. The test name claims it's testing the mapping but it isn't. The hbs test with no-bare-strings → ember/template-no-bare-strings correctly tests this. Fix the gjs test name or use an actual ember/template-* rule.
Should fix
4. Semantic mismatch with ember-template-lint
In ember-template-lint, {{! template-lint-disable }} disables from that point for the rest of the scope (until template-lint-enable). This PR gives it "next line only" semantics (like eslint-disable-next-line).
Users migrating from ember-template-lint will expect disable/enable pairs to work:
But only line 1 would be suppressed here.
Consider either:
- (a) Rename to
template-lint-disable-next-lineto avoid confusion, or - (b) Implement range-based
disable/enableto match ember-template-lint behavior, or - (c) Document the difference prominently
5. Regex doesn't support @ for scoped plugin rules
[\s\w,/-] can't match @typescript-eslint/no-unused-vars or @ember/no-classic-classes. Adding @ to the character class would fix this:
/{{!\s+template-lint-disable([\s\w,/@-]*)}}/g
Implements a `template-lint-disable` comment directive that suppresses
lint errors on the next line in template regions of gjs/gts/hbs files.
This works like `eslint-disable-next-line` but uses the template-lint
naming convention familiar to ember-template-lint users.
Supported comment formats:
{{! template-lint-disable }}
{{!-- template-lint-disable rule-name --}}
<!-- template-lint-disable rule-name -->
Rule names can be specified as either eslint rule IDs (e.g. `no-undef`)
or with `ember/template-` prefix (e.g. `ember/template-no-bare-strings`).
When no rule is specified, all rules are suppressed on the next line.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Only mustache comments ({{! ... }} and {{!-- ... --}}) are supported
for the template-lint-disable directive.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Apply the processor to hbs files in base configs so template-lint-disable works in standalone .hbs templates, not just gjs/gts files. Also handle template AST edge case where TextNodes start on the comment line. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Split the single regex with overlapping quantifiers into two separate patterns for mustache comments and mustache block comments. Each uses a specific character class ([\w\s,/-]*) that cannot cause polynomial backtracking. Also removes the -+$ cleanup regex since the block comment pattern now captures only the rules content without trailing dashes. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Reorder regex char class per unicorn/better-regex - Use for-of loop per unicorn/no-for-loop - Move initHbsESLint to outer scope per unicorn/consistent-function-scoping - Disable import/no-unresolved for ember-eslint-parser/hbs (valid export) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Revert recommended.mjs to original (cannot change recommended config) - Keep noop processor unchanged; export template-lint-disable as a separate processor (ember/template-lint-disable) to avoid breaking existing gjs/gts users - Wire hbs files to use the new processor by default in base configs (hbs had no prior config, so this is additive) - Move hbs tests to a separate hbs-parser-test.js file - Update gjs template-lint-disable tests to explicitly use the new processor via initESLintWithTemplateLintDisable() Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Cannot change existing configs; the template-lint-disable processor is available for users to opt into via their own config. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
No config changes in this PR; the template-lint-disable processor is exported for users to opt into in their own config. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
NullVoxPopuli-ai-agent
force-pushed
the
feat/template-lint-disable-pragma
branch
from
fcc9ee7 to
2df802f
Compare
🏎️ Benchmark Comparison
Full mitata output |
- Add early-exit in preprocess for files without directives (near-zero overhead for common case) - Add `@` to regex character classes to support scoped plugin rules like @typescript-eslint/no-unused-vars - Document semantic difference from ember-template-lint (next-line-only vs scope-based disable) and comment-line suppression behavior - Fix misleading test name that claimed to test ember/ prefix mapping but was actually testing exact rule ID matching Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
Re-review by claude: Round-1 items 1–5 all look addressed ✓. Re-running the review surfaced one thing we missed the first time. 🔴
|
- Only delegate to noop.postprocess for gjs/gts files — the hbs parser never calls registerParsedFile, so noop appends irrelevant gjs/gts setup instructions to hbs error messages - Add .message assertion in hbs test to catch future regressions - Add test for comment-line suppression (eslint-disable-line behavior) - Add test for @-scoped plugin rule names in regex Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Hoist gjs/gts extension regex to module level to avoid recompiling on every postprocess call - Remove duplicate "disables a rule matched by exact ESLint rule ID" test — identical to "disables a specific rule by eslint rule name" - Rename @-scoped test to accurately describe what it tests (regex parsing, not rule suppression) - Clarify doc comment: suppression covers both comment line and next line (not just "like eslint-disable-line") Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Explain why MUSTACHE_COMMENT_REGEX uses \s+ (required to avoid
ambiguity with {{!text}} syntax) while MUSTACHE_BLOCK_COMMENT_REGEX
uses \s* (dashes already delimit the comment)
- Drop misleading "like eslint-disable-line" from test name
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
re-review dump: 🔴
|
- Add regex lookahead to reject template-lint-disable-next-line and template-lint-disable-tree — these ember-template-lint directives previously matched silently and suppressed nothing - Add ember/ prefix mapping to matchesRule so template-no-bare-strings (the form used in plugin docs and filenames) matches correctly - Remove dead registerParsedFile re-export (parser imports it directly from ember-eslint-parser, not from the processor object) - Fix template literal lint violation in hbs test - Document unsupported directives (enable, disable-next-line, disable-tree) and JS string literal caveat in JSDoc Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
Everything seems to be in great shape, now, with two comments: The new processor still sits at plugin.processors['template-lint-disable'] with no shipped config referencing it (both lib/config/base.js and lib/config-legacy/base.js are unchanged from master and still set processor: 'ember/noop'), and there's no README update. A user following the plugin's setup guide will never discover this feature exists. The PR description still says "The processor is registered under the same noop key so all existing config references (ember/noop) continue to work unchanged", which hasn't matched the code since commits |
|
I'm not changing the configs without an RFC 🙈 I don't think the new processor will be used unless the configs are updated (which is ideal). I'll add some README stuff, but but with lots of disclaimers, I don't know if we want to have the same semver guarantees with the template-lint replacements until we have configs for them just yet |
Documents the experimental opt-in processor with setup instructions for both flat and legacy configs, usage examples, supported rule name forms, and differences from ember-template-lint semantics. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
okay, i think it is fine. |
- Add parser: 'ember-eslint-parser/hbs' to README config examples — required for hbs files but was missing - Document eslint-disable/eslint-enable as the way to suppress ranges (works natively via mustache comments, no special processor needed) - Add test verifying eslint-disable/eslint-enable range suppression works in hbs templates Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Standard ESLint directives (eslint-disable-next-line, eslint-disable/ eslint-enable ranges) work natively in mustache comments alongside the template-lint-disable processor. Add both forms to the README and add a test for eslint-disable-next-line in hbs. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Verifies that standard ESLint range directives via mustache comments work correctly in gjs templates — errors inside the disable/enable range are suppressed, errors after the range still fire. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
@NullVoxPopuli added an experiment of range-version here: NullVoxPopuli-ai-agent#3 – Supporting range disable could be important to be a simple migration for candidate for ember-template-lint Or maybe it is enough with next line + whole file disable? |
|
prototype of alternative codemod johanrd#3 |
4 participants
Summary
Adds a
template-lint-disablecomment directive that suppresses lint errors on the next line in template regions of gjs/gts/hbs files. Works likeeslint-disable-next-linebut uses thetemplate-lint-disablenaming convention familiar to ember-template-lint users.This processor is opt-in — it is registered as
ember/template-lint-disableand must be explicitly configured. The existingember/noopprocessor is unchanged.Setup
Add the processor to your ESLint config for the file types you want:
Usage
Rule names can be specified in three forms:
no-undef,ember/template-no-bare-stringstemplate-no-bare-strings(maps toember/template-no-bare-strings)no-bare-strings(maps toember/template-no-bare-strings)Differences from ember-template-lint
template-lint-disablesuppresses the next line (and the comment line itself). It does NOT disable for the rest of the scope — there is notemplate-lint-enablecounterpart.template-lint-disableis recognized:template-lint-disable-next-lineandtemplate-lint-disable-treeare not matched and will silently do nothing.<!-- -->) are not supported — only Handlebars mustache comments ({{! }}and{{!-- --}}).Implementation
Wraps the existing
noopprocessor with a new processor that:preprocess: fast-path skips files withouttemplate-lint-disablein the text; otherwise parses comments and records which lines/rules should be suppressedpostprocess: delegates tonoop.postprocessfor gjs/gts files (config validation), then filters out suppressed messagesFollow-up
.hbsfiles (which have no processor config yet)Test plan
{{! template-lint-disable }}{{!-- template-lint-disable --}}template-lint-disable-next-lineortemplate-lint-disable-treetemplate-no-bare-stringsmiddle form (ember/ prefix mapping).hbsfiles are not corrupted with gjs/gts setup instructions🤖 Generated with Claude Code