Skip to content
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

Merged
merged 13 commits into from
Dec 18, 2023

Conversation

kowalczyk-krzysztof
Copy link
Contributor

Refs #3517

Copy link
Member

@char0n char0n left a 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.

@kowalczyk-krzysztof
Copy link
Contributor Author

@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.

Copy link
Member

@char0n char0n left a 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.

@kowalczyk-krzysztof
Copy link
Contributor Author

kowalczyk-krzysztof commented Dec 15, 2023

Thanks for the comments. I misunderstood the structure of Path Item Object and made some wrong assumptions 🤦🏻 But now I see what I've missed.
I'll also add fixtures and tests for OAS2.0 and OAS3.1 and modify the OAS3.0 one to take care of the missing cases.

We cannot use semantic predicates here.

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.

@kowalczyk-krzysztof
Copy link
Contributor Author

I modified my implementation according to above comments and I added fixtures for different OAS versions.
I'm not sure how to get rid of those @ts-ignore though since I can't really make assertions as there are no "generic" (as in, valid for any OAS version) types.

Copy link
Member

@char0n char0n left a 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.

@char0n
Copy link
Member

char0n commented Dec 15, 2023

I'm not sure how to get rid of those @ts-ignore though since I can't really make assertions as there are no "generic" (as in, valid for any OAS version) types.

We can use lowest common denominator which is ObjectElement. It should resolve issues with @ts-ignore

@kowalczyk-krzysztof
Copy link
Contributor Author

Pushed the suggested changes. Thanks for the get suggestion, I completely forgot it exists. However, this suggestion:

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 parameter (can't assign a type here) and changing the line below to be ObjectElement[]

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.

Copy link
Member

@char0n char0n left a 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 ;]

@kowalczyk-krzysztof
Copy link
Contributor Author

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 😄
I learned a lot about ApiDOM in this PR and hopefully my future PRs will require less help.

@char0n char0n merged commit 0055a09 into main Dec 18, 2023
7 of 8 checks passed
@char0n char0n deleted the path-template-validate branch December 18, 2023 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants