-
Notifications
You must be signed in to change notification settings - Fork 3
Add rules for aep-142 with docs and tests #53
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.
Thank you for this PR! Overall it looks really good.
I think there are some problems with handling arrays -- I've suggested a few changes to the tests to suss those out.
I also want to point out that the current implementation misses date/time values passed in parameters and headers. The latter is an interesting case since AEP-142 isn't consistent with AEP-142 for headers (see #364). For parameters, I don't think there is an easy fix so I'm fine if we just create an issue for that and deal with it in a follow-on PR.
aep/0142.yaml
Outdated
message: '{{error}}' | ||
severity: warn | ||
formats: ['oas2', 'oas3'] | ||
given: '$..[?(@property.match(/_(time|times|date|seconds|millis|micros|nanos)$/))]' |
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 think this pattern could potentially generate some nasty false positives. This will help cut down on that:
given: '$..[?(@property.match(/_(time|times|date|seconds|millis|micros|nanos)$/))]' | |
given: '$..properties[?(@property.match(/_(time|times|date|seconds|millis|micros|nanos)$/))]' |
delete_time: { | ||
type: 'string', | ||
format: 'date-time', | ||
}, |
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 think the following should pass but generates an error
}, | |
}, | |
more_times: { | |
type: 'array', | |
items: { | |
type: 'string', | |
format: 'date-time', | |
}, | |
}, |
update_time: { | ||
type: 'string', | ||
format: 'date-time', | ||
}, |
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 think the following should pass but generates an error:
}, | |
}, | |
more_times: { | |
type: 'array', | |
items: { | |
type: 'string', | |
format: 'date-time', | |
}, | |
}, |
Thanks for the review! I've addressed both concerns you raised. Changes Made1. JSONPath Pattern (aep/0142.yaml:11) This makes the pattern more specific by only targeting actual schema properties, reducing potential false positives. 2. Array Handling for Per AEP-142, fields ending in
3. Test Coverage Note on test placement: I added the Test ResultsAll 28 AEP-142 tests passing ✓ Let me know if you'd like any adjustments! |
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 #46
Implements lint rules for AEP-142 (Time and Duration) to validate time-related fields in OpenAPI specifications.
Rules Implemented
_time
suffix for timestamp fieldsReference
Testing
test/0142/
Documentation
docs/0142.md
docs/rules.md
index