Skip to content

Commit ff23c7b

Browse files
heiskrheiskr
authored
Fix TODOCS linter rule to catch placeholders in frontmatter (#56248)
1 parent 0371bcb commit ff23c7b

4 files changed

Lines changed: 233 additions & 0 deletions

File tree

‎src/content-linter/scripts/lint-content.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -568,6 +568,7 @@ function getMarkdownLintConfig(errorsOnly, runRules) {
568568
const searchReplaceRules = []
569569
const dataSearchReplaceRules = []
570570
const ymlSearchReplaceRules = []
571+
const frontmatterSearchReplaceRules = []
571572

572573
for (const searchRule of ruleConfig.rules) {
573574
const searchRuleSeverity = getRuleSeverity(searchRule, isPrecommit)
@@ -579,6 +580,11 @@ function getMarkdownLintConfig(errorsOnly, runRules) {
579580
if (searchRule['yml-files']) {
580581
ymlSearchReplaceRules.push(searchRule)
581582
}
583+
// Add search-replace rules to frontmatter configuration for rules that make sense in frontmatter
584+
// This ensures rules like TODOCS detection work in frontmatter
585+
if (searchRule.applyToFrontmatter) {
586+
frontmatterSearchReplaceRules.push(searchRule)
587+
}
582588
}
583589

584590
if (searchReplaceRules.length > 0) {
@@ -593,6 +599,10 @@ function getMarkdownLintConfig(errorsOnly, runRules) {
593599
config.yml[ruleName] = { ...ruleConfig, rules: ymlSearchReplaceRules }
594600
if (customRule) configuredRules.yml.push(customRule)
595601
}
602+
if (frontmatterSearchReplaceRules.length > 0) {
603+
config.frontMatter[ruleName] = { ...ruleConfig, rules: frontmatterSearchReplaceRules }
604+
if (customRule) configuredRules.frontMatter.push(customRule)
605+
}
596606
continue
597607
}
598608

‎src/content-linter/style/github-docs.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,7 @@ export const searchReplaceConfig = {
342342
precommitSeverity: 'warning',
343343
'partial-markdown-files': true,
344344
'yml-files': true,
345+
applyToFrontmatter: true, // Critical for content quality - prevents placeholders in titles, intros, etc.
345346
},
346347
{
347348
name: 'docs-domain',
@@ -351,6 +352,7 @@ export const searchReplaceConfig = {
351352
severity: 'error',
352353
'partial-markdown-files': true,
353354
'yml-files': true,
355+
applyToFrontmatter: true, // Should not appear in frontmatter
354356
},
355357
{
356358
name: 'help-domain',
@@ -360,6 +362,7 @@ export const searchReplaceConfig = {
360362
severity: 'error',
361363
'partial-markdown-files': true,
362364
'yml-files': true,
365+
applyToFrontmatter: true, // Should not appear in frontmatter
363366
},
364367
{
365368
name: 'developer-domain',
@@ -373,6 +376,7 @@ export const searchReplaceConfig = {
373376
severity: 'error',
374377
'partial-markdown-files': true,
375378
'yml-files': true,
379+
applyToFrontmatter: true, // Should not appear in frontmatter
376380
},
377381
{
378382
// Catches usage of old liquid data reusable syntax. For example:
@@ -384,6 +388,7 @@ export const searchReplaceConfig = {
384388
severity: 'error',
385389
'partial-markdown-files': true,
386390
'yml-files': true,
391+
applyToFrontmatter: true, // Can appear in frontmatter strings
387392
},
388393
{
389394
// Catches usage of old octicon variable syntax. For example:
@@ -396,6 +401,7 @@ export const searchReplaceConfig = {
396401
severity: 'error',
397402
'partial-markdown-files': true,
398403
'yml-files': true,
404+
applyToFrontmatter: true, // Can appear in frontmatter strings
399405
},
400406
],
401407
},

‎src/content-linter/tests/unit/frontmatter-search-replace.js‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
import { describe, expect, test } from 'vitest'
2+
import markdownlint from 'markdownlint'
3+
import searchReplace from 'markdownlint-rule-search-replace'
4+
5+
import { searchReplaceConfig } from '../../style/github-docs'
6+
7+
describe('search-replace rule in frontmatter', () => {
8+
test('TODOCS placeholder in frontmatter is detected', async () => {
9+
const markdown = ['---', 'title: TODOCS', '---', '', 'Clean content.'].join('\n')
10+
11+
const result = await markdownlint.promises.markdownlint({
12+
frontMatter: null,
13+
strings: { markdown },
14+
config: {
15+
'search-replace': searchReplaceConfig['search-replace'],
16+
},
17+
customRules: [searchReplace],
18+
})
19+
20+
const errors = result.markdown || []
21+
22+
// Should find TODOCS in frontmatter
23+
const todosErrors = errors.filter((e) => e.errorDetail && /TODOCS/.test(e.errorDetail))
24+
expect(todosErrors.length).toBe(1)
25+
expect(todosErrors[0].lineNumber).toBe(2) // title: TODOCS
26+
})
27+
28+
test('multiple TODOCS in frontmatter are all detected', async () => {
29+
const markdown = [
30+
'---',
31+
'title: TODOCS',
32+
'shortTitle: TODOCS',
33+
'intro: TODOCS',
34+
'---',
35+
'',
36+
'Content without placeholder.',
37+
].join('\n')
38+
39+
const result = await markdownlint.promises.markdownlint({
40+
frontMatter: null,
41+
strings: { markdown },
42+
config: {
43+
'search-replace': searchReplaceConfig['search-replace'],
44+
},
45+
customRules: [searchReplace],
46+
})
47+
48+
const errors = result.markdown || []
49+
50+
// Should find all TODOCS instances in frontmatter
51+
const todosErrors = errors.filter((e) => e.errorDetail && /TODOCS/.test(e.errorDetail))
52+
expect(todosErrors.length).toBe(3)
53+
expect(todosErrors[0].lineNumber).toBe(2) // title: TODOCS
54+
expect(todosErrors[1].lineNumber).toBe(3) // shortTitle: TODOCS
55+
expect(todosErrors[2].lineNumber).toBe(4) // intro: TODOCS
56+
})
57+
58+
test('domain rules work in frontmatter', async () => {
59+
const markdown = [
60+
'---',
61+
'title: Check docs.github.com for info',
62+
'shortTitle: Visit help.github.com',
63+
'intro: See developer.github.com for API docs',
64+
'---',
65+
'',
66+
'Content without domain references.',
67+
].join('\n')
68+
69+
const result = await markdownlint.promises.markdownlint({
70+
frontMatter: null,
71+
strings: { markdown },
72+
config: {
73+
'search-replace': searchReplaceConfig['search-replace'],
74+
},
75+
customRules: [searchReplace],
76+
})
77+
78+
const errors = result.markdown || []
79+
80+
// Should find domain errors in frontmatter
81+
const domainErrors = errors.filter(
82+
(e) => e.errorDetail && /docs-domain|help-domain|developer-domain/.test(e.errorDetail),
83+
)
84+
expect(domainErrors.length).toBe(3)
85+
expect(domainErrors[0].lineNumber).toBe(2) // docs domain in title
86+
expect(domainErrors[1].lineNumber).toBe(3) // help domain in shortTitle
87+
expect(domainErrors[2].lineNumber).toBe(4) // developer domain in intro
88+
})
89+
90+
test('deprecated liquid syntax in frontmatter is detected', async () => {
91+
const markdown = [
92+
'---',
93+
'title: "{{ site.data.variables.product.product_name }}"',
94+
'intro: "Use {{ octicon-plus An icon }} here"',
95+
'---',
96+
'',
97+
'Clean content.',
98+
].join('\n')
99+
100+
const result = await markdownlint.promises.markdownlint({
101+
frontMatter: null,
102+
strings: { markdown },
103+
config: {
104+
'search-replace': searchReplaceConfig['search-replace'],
105+
},
106+
customRules: [searchReplace],
107+
})
108+
109+
const errors = result.markdown || []
110+
111+
// Should find deprecated syntax errors in frontmatter
112+
const deprecatedErrors = errors.filter(
113+
(e) => e.errorDetail && /site\.data|octicon/.test(e.errorDetail),
114+
)
115+
expect(deprecatedErrors.length).toBe(2)
116+
expect(deprecatedErrors[0].lineNumber).toBe(2) // site.data syntax
117+
expect(deprecatedErrors[1].lineNumber).toBe(3) // octicon syntax
118+
})
119+
})

‎src/content-linter/tests/unit/search-replace.js‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,4 +60,102 @@ describe(searchReplace.names.join(' - '), () => {
6060
const errors = result.markdown
6161
expect(errors.length).toBe(4)
6262
})
63+
64+
test('TODOCS placeholder in frontmatter causes errors when frontmatter is included', async () => {
65+
const markdown = [
66+
'---',
67+
'title: TODOCS',
68+
'shortTitle: TODOCS',
69+
'intro: TODOCS',
70+
'versions:',
71+
' ghec: "*"',
72+
'---',
73+
'',
74+
'This is content that has no placeholder.',
75+
].join('\n')
76+
const result = await runRule(searchReplace, {
77+
strings: { markdown },
78+
ruleConfig: searchReplaceConfig['search-replace'],
79+
markdownlintOptions: { frontMatter: null }, // Include frontmatter in linting
80+
})
81+
const errors = result.markdown
82+
// Should find 3 TODOCS occurrences in frontmatter
83+
expect(errors.length).toBe(3)
84+
expect(errors[0].lineNumber).toBe(2) // title: TODOCS
85+
expect(errors[1].lineNumber).toBe(3) // shortTitle: TODOCS
86+
expect(errors[2].lineNumber).toBe(4) // intro: TODOCS
87+
})
88+
89+
test('TODOCS placeholder in both frontmatter and content', async () => {
90+
const markdown = [
91+
'---',
92+
'title: TODOCS',
93+
'intro: TODOCS',
94+
'---',
95+
'',
96+
'This content has TODOCS placeholder.',
97+
'And another TODOCS here.',
98+
].join('\n')
99+
const result = await runRule(searchReplace, {
100+
strings: { markdown },
101+
ruleConfig: searchReplaceConfig['search-replace'],
102+
markdownlintOptions: { frontMatter: null }, // Include frontmatter in linting
103+
})
104+
const errors = result.markdown
105+
// Should find 4 TODOCS occurrences total (2 in frontmatter + 2 in content)
106+
expect(errors.length).toBe(4)
107+
expect(errors[0].lineNumber).toBe(2) // title: TODOCS
108+
expect(errors[1].lineNumber).toBe(3) // intro: TODOCS
109+
expect(errors[2].lineNumber).toBe(6) // content TODOCS
110+
expect(errors[3].lineNumber).toBe(7) // content TODOCS
111+
})
112+
113+
test('TODOCS placeholder in frontmatter is not caught with default frontmatter handling', async () => {
114+
const markdown = [
115+
'---',
116+
'title: TODOCS',
117+
'shortTitle: TODOCS',
118+
'intro: TODOCS',
119+
'versions:',
120+
' ghec: "*"',
121+
'---',
122+
'',
123+
'This is content that has no placeholder.',
124+
].join('\n')
125+
const result = await runRule(searchReplace, {
126+
strings: { markdown },
127+
ruleConfig: searchReplaceConfig['search-replace'],
128+
// Default frontmatter handling (frontmatter is stripped from content)
129+
})
130+
const errors = result.markdown
131+
// When using default frontmatter handling (frontmatter is stripped from content),
132+
// this unit test only tests the search-replace rule in isolation on the content portion.
133+
// Frontmatter linting happens separately in the actual linting system.
134+
expect(errors.length).toBe(0)
135+
})
136+
137+
test('TODOCS in frontmatter is detected when frontmatter is included in content', async () => {
138+
// This test shows that search-replace works on frontmatter when it's included in content
139+
const frontmatterOnly = [
140+
'---',
141+
'title: TODOCS',
142+
'shortTitle: TODOCS',
143+
'intro: TODOCS',
144+
'---',
145+
].join('\n')
146+
147+
// When frontmatter is treated as content, search-replace works
148+
const result = await runRule(searchReplace, {
149+
strings: { markdown: frontmatterOnly },
150+
ruleConfig: searchReplaceConfig['search-replace'],
151+
markdownlintOptions: { frontMatter: null }, // Include frontmatter in content
152+
})
153+
const errors = result.markdown
154+
155+
// Finds all 3 TODOCS in frontmatter when frontmatter is included in content
156+
expect(errors.length).toBe(3)
157+
expect(errors[0].lineNumber).toBe(2) // title: TODOCS
158+
expect(errors[1].lineNumber).toBe(3) // shortTitle: TODOCS
159+
expect(errors[2].lineNumber).toBe(4) // intro: TODOCS
160+
})
63161
})

0 commit comments

Comments
 (0)