-
Notifications
You must be signed in to change notification settings - Fork 1
[proposal] Check for missed opportunities to use subs #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds inline warnings for missed opportunities to use substitutions (subs
) when hardcoded values are detected that match available substitution keys. The goal is to encourage consistent use of substitutions across documentation.
- Creates a centralized substitution management system with validation warnings
- Adds product definitions and integrates them with frontmatter validation
- Consolidates duplicate substitution parsing logic across multiple providers
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
src/substitutions.ts | New centralized substitution management with caching and product integration |
src/substitutionValidationProvider.ts | New validation provider that warns when hardcoded values could use subs |
src/substitutionHoverProvider.ts | Refactored to use centralized substitution logic |
src/substitutionCompletionProvider.ts | Refactored to use centralized substitution logic |
src/products.ts | New centralized product definitions for validation and substitutions |
src/frontmatterSchema.ts | Updated to use centralized product list for validation |
src/extension.ts | Integrated substitution validation into the extension lifecycle |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
} | ||
|
||
private validateContent(errors: ValidationError[], document: vscode.TextDocument): void { | ||
const lines = document.getText().split('\n') |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon at end of statement.
const lines = document.getText().split('\n') | |
const lines = document.getText().split('\n'); |
Copilot uses AI. Check for mistakes.
const regex = new RegExp(`(\\W|^)${value}(\\W|$)`, 'gm') | ||
const matches = line.match(regex) |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern may fail with special regex characters in substitution values. The value
should be escaped using a regex escape function to prevent regex injection issues.
Copilot uses AI. Check for mistakes.
const regex = new RegExp(`(\\W|^)${value}(\\W|$)`, 'gm') | ||
const matches = line.match(regex) | ||
if (matches) { | ||
for (const match of matches) { | ||
const lineNumber = i | ||
const startChar = line.indexOf(match) > 0 | ||
? line.indexOf(match) + 1 | ||
: 0 | ||
const endChar = startChar + value.length | ||
const range = new vscode.Range(lineNumber, startChar, lineNumber, endChar); | ||
if (!errors.find(err => err.range.contains(range))) { | ||
errors.push({ | ||
range, | ||
message: `Use substitute \`{{${key}}}\` instead of \`${value}\``, | ||
severity: vscode.DiagnosticSeverity.Warning, | ||
code: 'use_sub' | ||
}); | ||
} |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for calculating startChar
is incorrect. When indexOf(match) > 0
, it adds 1 to skip the word boundary character, but when indexOf(match) === 0
, it should still account for the actual position of the value within the match. This will cause incorrect highlighting positions.
const regex = new RegExp(`(\\W|^)${value}(\\W|$)`, 'gm') | |
const matches = line.match(regex) | |
if (matches) { | |
for (const match of matches) { | |
const lineNumber = i | |
const startChar = line.indexOf(match) > 0 | |
? line.indexOf(match) + 1 | |
: 0 | |
const endChar = startChar + value.length | |
const range = new vscode.Range(lineNumber, startChar, lineNumber, endChar); | |
if (!errors.find(err => err.range.contains(range))) { | |
errors.push({ | |
range, | |
message: `Use substitute \`{{${key}}}\` instead of \`${value}\``, | |
severity: vscode.DiagnosticSeverity.Warning, | |
code: 'use_sub' | |
}); | |
} | |
const regex = new RegExp(`(\\W|^)${value}(\\W|$)`, 'g') | |
let match; | |
while ((match = regex.exec(line)) !== null) { | |
const lineNumber = i; | |
// match[1] is the leading boundary, match.index is the start of the match | |
const startChar = match.index + (match[1] ? match[1].length : 0); | |
const endChar = startChar + value.length; | |
const range = new vscode.Range(lineNumber, startChar, lineNumber, endChar); | |
if (!errors.find(err => err.range.contains(range))) { | |
errors.push({ | |
range, | |
message: `Use substitute \`{{${key}}}\` instead of \`${value}\``, | |
severity: vscode.DiagnosticSeverity.Warning, | |
code: 'use_sub' | |
}); |
Copilot uses AI. Check for mistakes.
const filteredSubs = Object.keys(unorderedSubs).filter(sub => { | ||
return !Object.values(PRODUCTS).includes(unorderedSubs[sub]) |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Object.values(PRODUCTS).includes()
call inside the filter creates a new array on every iteration. Consider creating the values array once outside the loop for better performance.
const filteredSubs = Object.keys(unorderedSubs).filter(sub => { | |
return !Object.values(PRODUCTS).includes(unorderedSubs[sub]) | |
const productValues = Object.values(PRODUCTS); | |
const filteredSubs = Object.keys(unorderedSubs).filter(sub => { | |
return !productValues.includes(unorderedSubs[sub]) |
Copilot uses AI. Check for mistakes.
for (let i = 0; i < lines.length; i++) { | ||
const line = lines[i]; |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The loop uses indexed iteration but the index i
is only used to get the line. Consider using for (const line of lines)
for cleaner, more readable code.
for (let i = 0; i < lines.length; i++) { | |
const line = lines[i]; | |
for (const line of lines) { |
Copilot uses AI. Check for mistakes.
@colleenmcginnis This is a great idea. It would indeed be fantastic to pull the data directly from docs-builder — this is why I created this proposal here: elastic/docs-builder#1988 Could you have a look at Copilot's comments before I test this myself later this week? Thank you! |
In addition to autocompletion for
subs
, this would also add inline warnings when an author hard-codes the value of a sub instead of using the sub key. In my opinion,subs
aren't very helpful unless we use them consistently. This would be a light-touch way to encourage consistent use ofsubs
and weed out unnecessarysubs
without introducing a bunch of errors.In addition to the inline warnings, this PR would also:
src/products.ts
that can be used to both validateproducts
frontmatter insrc/frontmatterSchema.ts
and suggest usingproduct
subs.subs
insrc/substitutions.ts
instead of duplicating the logic insrc/substitutionCompletionProvider.ts
andsrc/substitutionHoverProvider.ts
.subs
in thedocset.yml
file in the local workspace with the list ofproduct
subs.Some notes about the logic for choosing which sub to recommend if there are multiple matches:
docset.yml
file has the same value as aproduct
sub, preference goes to theproduct
sub.Fleet
/Fleet Server
orElastic Agent
/Elastic Agents
), preference goes to the longer string.@theletterf let me know what you think. 🙂