Improving and unifying oas ruleset #772
Conversation
| formats: ["oas3"] | ||
| given: "$" | ||
| given: "$.servers" |
There was a problem hiding this comment.
Noticed a few of these. We want to get as close to the field as possible to make it show the right line on Studio.
There was a problem hiding this comment.
$ combined with server field is fully compliant with $.servers, so we need to be careful about changing them.
The biggest difference is that a function won't be run if property matching $.servers does not exist in the document, while it'd be run if field was specified.
cc @nulltoken since you are working on that too.
I'm pretty sure src/rulesets/oas3/__tests__/api-servers.ts fails now.
There was a problem hiding this comment.
That said, we could simply add given to the UI. It's not a big chunk of work for me, and I'd feel a bit safer. 😅. We're all working on a huge chunk of work around OpenAPI rulesets, hence it might be trivial to sneak a small regression in, as there is plenty of code to review.
Last but not least, feels like a good candidate to be documented? The different is quite subtle from the ruleset perspective, but the behavior does differ quite a bit IMHO.
There was a problem hiding this comment.
@philsturgeon As #773 takes over this PR, would you agree to close this one and move the discussion over to #773?
cc @nulltoken since you are working on that too.
I'm pretty sure src/rulesets/oas3/tests/api-servers.ts fails now.
@P0lip I don't see it failing in #773. Or is it passing for the wrong reason? Or did I misunderstand you?
There was a problem hiding this comment.
@nulltoken I think the plan is to merge #773 into here then the two of you can continue to make progress over here.
There was a problem hiding this comment.
@philsturgeon I'm afraid this won't work as this PR is linked to a branch in the core repository. And I'm not granted with push bit in this one.
I've already cherry picked your commits in my branch and switched the base of #773 to target develop.
It should be safe to close this one and move over to #773. Your work wouldn't be lost and @P0lip and I could work there.
Thoughts?
There was a problem hiding this comment.
Sounds good, good luck!
philsturgeon
force-pushed
the
feat/improve-oas-rulesets
branch
from
5904907
to
7fcdc56
Compare
Fixes #725
Fixes #771
Checklist
Does this PR introduce a breaking change?
Trying to do this murdered my brain, but that might be because I was up until 2am trying.
Merging the rulesets is rather hard, especially as now the format detection is more important, and our tests dont have anything which trigger the rulesets. I added
swagger: 2.0oropenapi: 3.0to a bunch of them foolishly because all i had to do was force the format, which I have also done but am still getting errors.This is further confused by the many times I had to override
typeorseverityassetRulesTypeScript demands a proper constant instead of a string like "error". Can we normalize this on setRules, delete all the overrides, and figure out the format issue?I cannot dedicate more time to this one but its pretty bloomin close I think.