feat: improve changes issues positions#313
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #313 +/- ##
==========================================
+ Coverage 48.90% 49.70% +0.79%
==========================================
Files 82 83 +1
Lines 6825 6949 +124
Branches 277 295 +18
==========================================
+ Hits 3338 3454 +116
- Misses 3484 3492 +8
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the invalid-change-version linter rule to leverage the YAML AST for pinpoint issue locations, adds supporting YAML utilities, and updates tests and constants to match the new behavior.
- Introduce YAML AST helpers (
findPropertyByName,normalizeNode,createYamlIssueReporter) - Refactor
invalid-change-versionto parse withyaml'sLineCounterand report precise lines - Update tests and add a new lint message for invalid change property types
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/parser/index.mjs | Adjusted JSDoc to accept import('mdast').Node instead of Html |
| src/linter/utils/yaml.mjs | New YAML utilities for property lookup, normalization, and issue reporting |
| src/linter/rules/invalid-change-version.mjs | Refactored rule to use AST parsing and createYamlIssueReporter |
| src/linter/rules/tests/invalid-change-version.test.mjs | Updated tests for new error positions and added invalid-property case |
| src/linter/constants.mjs | Added invalidChangeProperty lint message |
Comments suppressed due to low confidence (3)
src/linter/utils/yaml.mjs:40
- [nitpick] Make the error message more descriptive by including the actual node type encountered (e.g., using
node.type), rather than hard-coding "map".
throw new Error(`Unexpected node type: map`);
src/linter/utils/yaml.mjs:50
- There are currently no unit tests for
createYamlIssueReporter. Consider adding tests that verify correct line calculations based on a mockLineCounterand sample nodes.
export const createYamlIssueReporter = (yamlNode, lineCounter) => {
src/linter/rules/tests/invalid-change-version.test.mjs:72
- [nitpick] The variable name
first_calluses snake_case; consider using camelCase (e.g.,firstCall) to stay consistent with project style.
const first_call = context.report.mock.calls[0];
AugustinMauroy
left a comment
There was a problem hiding this comment.
LGMT ! just try to cover all line with test
|
bump @nodejs/web-infra |
|
I wonder why CI is green 🤔? #318 |
|
Is the test is correctly run ? |
Description
Improve
invalid-change-versionto provide users with more precise issue locations instead of only the entire YAML node position.Validation
Related Issues
Check List
node --run testand all tests passed.node --run format&node --run lint.