-
Notifications
You must be signed in to change notification settings - Fork 9
feat: improve changes issues positions #313
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
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.71% +0.80%
==========================================
Files 82 83 +1
Lines 6825 6960 +135
Branches 277 295 +18
==========================================
+ Hits 3338 3460 +122
- Misses 3484 3497 +13
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 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-version
to parse withyaml
'sLineCounter
and 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 mockLineCounter
and sample nodes.
export const createYamlIssueReporter = (yamlNode, lineCounter) => {
src/linter/rules/tests/invalid-change-version.test.mjs:72
- [nitpick] The variable name
first_call
uses snake_case; consider using camelCase (e.g.,firstCall
) to stay consistent with project style.
const first_call = context.report.mock.calls[0];
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 ! just try to cover all line with test
|
||
it('should report an issue if changes is not a sequence', () => { | ||
const yamlContent = dedent` | ||
<!-- YAML |
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 purpose of using dedentr, is that you can add spaces/tabs to align the lines with the variable definition padding level.
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.
const yamlContent = dedent`
<!-- YAML
changes:
abc:
def:
-->`;
Description
Improve
invalid-change-version
to provide users with more precise issue locations instead of only the entire YAML node position.Validation
Related Issues
Check List
node --run test
and all tests passed.node --run format
&node --run lint
.