-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat(apidom-ls): path template match parameter object lint #3538
Conversation
678e3c0
to
c272193
Compare
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.
Left one bigger comment with the draft of possible semantic implementation without doing ApiDOM -> POJO conversions.
@char0n I changed my implementation to be more semantic. I also modified the fixture to include a reference object and it seems that the linter function works on dereferenced specification so we don't need to handle that. |
7b88bdd
to
bf869f3
Compare
bf869f3
to
5dcdf03
Compare
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.
We're getting there! Please have a look at my CR comments. I'll duble-check the Reference Object
handling when we have current CR comments handled.
packages/apidom-ls/test/fixtures/validation/oas/path-template-all-defined.yaml
Outdated
Show resolved
Hide resolved
packages/apidom-ls/test/fixtures/validation/oas/path-template-all-defined.yaml
Outdated
Show resolved
Hide resolved
Thanks for the comments. I misunderstood the structure of
Yeah I see, Intellisense was showing me a lot of different import options and I got confused and I choose one at random, I wasn't aware that those predicate functions differ. I also didn't know about the polymorphic check option. |
I modified my implementation according to above comments and I added fixtures for different OAS versions. |
2778049
to
9d61642
Compare
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.
Some final nitpicks and possible enhancements.
We can use lowest common denominator which is |
bfa1617
to
169f0d6
Compare
Pushed the suggested changes. Thanks for the parameterElements.forEach((parameter: ObjectElement) => {
if (allowedLocation.includes(toValue(parameter.get('in')))) {
pathTemplateResolveParams[toValue(parameter.get('name')) as string] = 'placeholder';
}
}); doesn't work unfortunately, I still need to cast const parameterElements: Element[] = []; requires type casting in other places so I think this: parameterElements.forEach((parameter) => {
if (allowedLocation.includes(toValue((parameter as ObjectElement).get('in')))) {
pathTemplateResolveParams[toValue((parameter as ObjectElement).get('name'))] =
'placeholder';
}
}); is the cleanest looking solution. |
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.
All right, we're looking good! Nice job!
WARNING: please don't merge, I'll need to double check the Reference Object
handling and verify that we're working on dereferenced structure. After that I'll do the "sqash and merge".
Thanks for your patience ;]
Thanks for all the comments and suggestions 😄 |
Refs #3517