-
Notifications
You must be signed in to change notification settings - Fork 4
Add rules for aep-122 with docs and tests #54
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
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 implements comprehensive linting rules for AEP-122 (Resource Paths) to validate OpenAPI specifications against resource path conventions. The implementation includes 7 lint rules covering path field validation, collection identifier formatting, resource ID types, and field naming conventions.
Key changes:
- Added 7 new AEP-122 lint rules with complete Spectral YAML configuration
- Implemented 24 comprehensive test cases covering all rules and edge cases
- Created detailed documentation explaining each rule with examples and disabling instructions
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| aep/0122.yaml | Spectral rule definitions for all 7 AEP-122 linting rules |
| spectral.yaml | Added reference to new 0122 ruleset |
| docs/0122.md | Comprehensive documentation for all AEP-122 rules with examples |
| docs/rules.md | Added link to AEP-122 documentation |
| test/0122/*.test.js | 7 test files with 24 total test cases covering all rules |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| beforeAll(async () => { | ||
| linter = await linterForAepRule('0122', 'aep-122-resource-path-field'); | ||
| return linter; |
Copilot
AI
Oct 19, 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 return linter; statement on line 8 is unnecessary in beforeAll. The async function already resolves with the linter value when the promise completes, and the linter is stored in the outer scope variable. This line should be removed.
| return linter; |
test/0122/resource-id-type.test.js
Outdated
|
|
||
| beforeAll(async () => { | ||
| linter = await linterForAepRule('0122', 'aep-122-resource-id-type'); | ||
| return linter; |
Copilot
AI
Oct 19, 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 return linter; statement on line 8 is unnecessary in beforeAll. The async function already resolves with the linter value when the promise completes, and the linter is stored in the outer scope variable. This line should be removed.
| return linter; |
|
|
||
| beforeAll(async () => { | ||
| linter = await linterForAepRule('0122', 'aep-122-path-field-required'); | ||
| return linter; |
Copilot
AI
Oct 19, 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 return linter; statement on line 8 is unnecessary in beforeAll. The async function already resolves with the linter value when the promise completes, and the linter is stored in the outer scope variable. This line should be removed.
| return linter; |
test/0122/parent-field-type.test.js
Outdated
|
|
||
| beforeAll(async () => { | ||
| linter = await linterForAepRule('0122', 'aep-122-parent-field-type'); | ||
| return linter; |
Copilot
AI
Oct 19, 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 return linter; statement on line 8 is unnecessary in beforeAll. The async function already resolves with the linter value when the promise completes, and the linter is stored in the outer scope variable. This line should be removed.
| return linter; |
test/0122/no-self-links.test.js
Outdated
|
|
||
| beforeAll(async () => { | ||
| linter = await linterForAepRule('0122', 'aep-122-no-self-links'); | ||
| return linter; |
Copilot
AI
Oct 19, 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 return linter; statement on line 8 is unnecessary in beforeAll. The async function already resolves with the linter value when the promise completes, and the linter is stored in the outer scope variable. This line should be removed.
| return linter; |
test/0122/no-path-suffix.test.js
Outdated
|
|
||
| beforeAll(async () => { | ||
| linter = await linterForAepRule('0122', 'aep-122-no-path-suffix'); | ||
| return linter; |
Copilot
AI
Oct 19, 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 return linter; statement on line 8 is unnecessary in beforeAll. The async function already resolves with the linter value when the promise completes, and the linter is stored in the outer scope variable. This line should be removed.
| return linter; |
|
|
||
| beforeAll(async () => { | ||
| linter = await linterForAepRule('0122', 'aep-122-collection-identifier-kebab-case'); | ||
| return linter; |
Copilot
AI
Oct 19, 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 return linter; statement on line 8 is unnecessary in beforeAll. The async function already resolves with the linter value when the promise completes, and the linter is stored in the outer scope variable. This line should be removed.
| return linter; |
|
|
||
| beforeAll(async () => { | ||
| linter = await linterForAepRule('0122', 'aep-122-collection-identifier-format'); | ||
| return linter; |
Copilot
AI
Oct 19, 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 return linter; statement on line 8 is unnecessary in beforeAll. The async function already resolves with the linter value when the promise completes, and the linter is stored in the outer scope variable. This line should be removed.
| return linter; |
Fixed Issue #1: aep-122-no-path-suffix scopeThe rule was checking ALL schemas, causing false positives for legitimate _path suffixes in non-AEP schemas (e.g., Changes:
Test results: ✅ 3/3 passing (added 1 new test) Commit: 479568e |
Fixed Issue #2: Add deep nesting testThe existing tests only validated 2 levels of nesting ( Changes:
Test results: ✅ 26/26 passing (added 1 new test) Commit: a6bd8c1 |
Fixed Issue #5: Remove unnecessary return linterCopilot's automated review was correct - the Why it's unnecessary:
Changes:
Test results: ✅ 26/26 passing Commit: 03b6307 |
Fixed Issue #6: Simplify kebab-case regexThe original regex was overly complex with nested groups, alternation, and redundant patterns. Old regex: Why this is better:
Follows best practices:
Test results: ✅ 26/26 passing (behavior unchanged) Commit: f563fe0 |
Fixed: example.oas.yaml AEP-122 violationsThe CI was failing because Changes:
Verification: Spec requirement confirmed: Commit: 95ce766 |
95ce766 to
1a2369f
Compare
Fixed: example.oas.yaml AEP-122 complianceThe example file had pre-existing AEP-122 violations that were exposed by our new linting rules. Since we introduced these rules, we''ve updated the example to comply. Changes made:
Results: Why this matters: Commit: c9bf099 |
mkistler
left a comment
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.
Thank you for this PR. It looks really good and has just a few small problems that I've noted in the comments.
We'll need to get a definite answer on whether path should be required before moving forward with this.
|
Please drop the rule that checks that "path" is required. This appears to be a misinterpretation (understandable!) of the language in the AEP. |
Ack, done. |
mkistler
left a comment
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.
Looking very good but I flagged a few more small changes to make.
mkistler
left a comment
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.
Looks good! 👍
Closes #48
Implements lint rules for AEP-122 (Resource Paths) to validate resource path structure in OpenAPI specifications.
Rules Implemented
Reference
Testing
Documentation