-
Notifications
You must be signed in to change notification settings - Fork 10
feat: introduce raw mdast linter #275
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
Conversation
CC @nodejs/web-infra |
Co-authored-by: Aviv Keller <[email protected]>
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.
LGMT !
I didn't had time to review this PR, so giving a review to be acted as a follow-up. |
const lint = (file, tree) => { | ||
const context = createContext(file, tree); | ||
|
||
for (const rule of rules) { |
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.
nit: Why not forEach?
|
||
return issues; | ||
// Process blockquotes to find stability nodes | ||
if (node.type === 'blockquote') { |
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.
Way too many nested if statements. This is really not ideal. If you have more than 2-3 nested statements this shows an underlying issue.
Reading and understanding this code becomes extremely hard. Please refactor and ensure you don't need all these statements.
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.
Also, linting shouldn't not be something complex and resource intensive, wonder if this linting rule is strictly necessary and not accidentally too complex.
.forEach(version => | ||
context.report({ | ||
level: 'error', | ||
message: version |
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.
Assign this to a constant, quite hard to read.
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.
I feel we need more documentation around this + how to contribute and create new linting rules.
I do believe we should introduce an interface/factory system for linting rules and normalizing this. It feels like we're overcomplicating things and some of the code has barely any documentation and barely explains what it is doing and why. This sort of code needs to be easy to read and ton understand and the linting rules need to be natively friendly for folks to understand how they work and why.
If possible please do an overhaul, cleanup, inline documentation, contribution documentation/guidelines + explaining how the linting system works.
I'd also appreciate a benchmark of just linting and then linting + normal run.
Description
Refactors the linter engine to operate directly on raw AST nodes, removing the use of metadata entries.
This expands future linting capabilities and enables more precise issue location reporting.
Validation
Related Issues
Related #271
Check List
node --run test
and all tests passed.node --run format
&node --run lint
.