-
Notifications
You must be signed in to change notification settings - Fork 91
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
remove broken discriminator rule in favor of new spectral rule #367
Conversation
This rule verifies that the 'propertyName' value of the discriminator is present in each subschema.
This rule was buggy and not previously configurable. The same rule with identical (albeit fixed) behavior is now available in our Spectral ruleset under the name 'discriminator' and is configurable.
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 great!
expect(visitedSchemas[0]).toBe( | ||
'paths./schema.get.responses.200.content.application/json.schema' | ||
); |
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.
Should we do this style instead?
expect(visitedSchemas).toContain(
'paths./schema.get.responses.200.content.application/json.schema'
);
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 intentionally chose not to use that style in order to make the test say "we found these schemas and only these schemas". In essence, to verify that there were no false positives in addition to no missing positives. I suppose my test still doesn't fully capture that though, because there is no check on the length of the array. I'll add that now
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.
Okay, sounds good!
# @ibm-cloud/openapi-ruleset [0.2.0](https://github.com/IBM/openapi-validator/compare/@ibm-cloud/[email protected]...@ibm-cloud/[email protected]) (2022-02-04) ### Features * add new spectral `discriminator` rule and remove old rule ([#367](#367)) ([390afc9](390afc9))
# [0.54.0](https://github.com/IBM/openapi-validator/compare/[email protected]@0.54.0) (2022-02-04) ### Features * add new spectral `discriminator` rule and remove old rule ([#367](#367)) ([390afc9](390afc9))
🎉 This PR is included in version 0.2.0 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 0.54.0 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
This rule was buggy and not previously configurable. The same rule with
identical (albeit fixed) behavior is now available in our Spectral ruleset
under the name 'discriminator' and is configurable.
I also did a lot of refactoring to reduce duplication and make it easier to add new
rules to our spectral ruleset.
Resolves #360