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

remove broken discriminator rule in favor of new spectral rule #367

Merged
merged 7 commits into from
Feb 4, 2022

Conversation

dpopp07
Copy link
Member

@dpopp07 dpopp07 commented Feb 1, 2022

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

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.
@dpopp07 dpopp07 requested review from hudlow and padamstx February 1, 2022 17:34
packages/ruleset/src/collections/index.js Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Member

@padamstx padamstx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@dpopp07 dpopp07 requested a review from hudlow February 3, 2022 22:54
@dpopp07 dpopp07 requested a review from hudlow February 4, 2022 18:38
Comment on lines +29 to +31
expect(visitedSchemas[0]).toBe(
'paths./schema.get.responses.200.content.application/json.schema'
);
Copy link
Member

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'
    );

Copy link
Member Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sounds good!

@dpopp07 dpopp07 requested a review from hudlow February 4, 2022 20:07
@dpopp07 dpopp07 merged commit 390afc9 into main Feb 4, 2022
@dpopp07 dpopp07 deleted the dp/discriminator-rule-conversion branch February 4, 2022 20:51
ibm-devx-sdk pushed a commit that referenced this pull request Feb 4, 2022
# @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))
ibm-devx-sdk pushed a commit that referenced this pull request Feb 4, 2022
# [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))
@ibm-devx-sdk
Copy link

🎉 This PR is included in version 0.2.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@ibm-devx-sdk
Copy link

🎉 This PR is included in version 0.54.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] False positive: "The discriminator property name used must be defined in this schema"
4 participants