-
-
Notifications
You must be signed in to change notification settings - Fork 248
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(oas): merge example rules #1313
Conversation
@P0lip Great job! One thought though. There are a lot of code duplication among the tests, and some tests exist in some file and not in others ( What would you think of extracting the valid/invalid schemas and the matching props of the results we'd like to check in one global collection, and then "inject" the schemas in proper locations of base documents in each test? We may have to tweak the expectations regarding the resulting paths, but it's possible this would greatly reduce the size of the diff and (maybe) make those tests a bit easier to maintain and reason about? |
Co-authored-by: nulltoken <[email protected]>
Co-authored-by: nulltoken <[email protected]>
Co-authored-by: nulltoken <[email protected]>
Co-authored-by: nulltoken <[email protected]>
Agree re duplication. It bothers me too.
Hm, not sure I'm following. What do you exactly suggest? |
There's a few approaches to avoid duplication:
I'm usually quite content with 1 or 2 but 3 can lead to awkward changes when multiple people do different things with the fixtures. Also keep in mind we don't need to fix this now, if we keep on boy scouting as we go it'll get better over time. |
@P0lip My thoughts were around a combination of @philsturgeon points 1 & 2. However, I'm also fine with the |
@nulltoken @philsturgeon alright, did some deduplication. |
This is a follow-up PR.
See #1284 (comment) for the reasoning.
tl;dr we no longer need such granular rules, as
given
can be an array of paths now.Checklist
Does this PR introduce a breaking change?