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

fix(core): consider message when de-duplicating results #2052

Merged
merged 3 commits into from
Feb 11, 2022
Merged

fix(core): consider message when de-duplicating results #2052

merged 3 commits into from
Feb 11, 2022

Conversation

dpopp07
Copy link
Contributor

@dpopp07 dpopp07 commented Feb 7, 2022

Before this change, the code and path of the results were the primary factors
considered in de-duplication. Results that came from the same rule and same OpenAPI
artifact but had different messages were de-duplicated and not all unique results
were returned.

This change updates the logic to take message into account so that all unique
results are displayed to the user.

I added a new test to confirm the behavior. I also updated an existing test - one of the tests resulted in multiple results returned for the same OpenAPI artifact with different error messages. I looked into addressing the extra rules but in this case, they are a little bit redundant and so I updated the tests to make the additional results expected. Let me know how you feel about this as an approach.

I also removed the outdated docs about custom de-duplication logic.

Fixes #2038

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Before this change, the `code` and `path` of the results were the primary factors
considered in de-duplication. Results that came from the same rule and same OpenAPI
artifact but had different messages were de-duplicated and not all unique results
were returned.

This change updates the logic to take `message` into account so that all unique
results are displayed to the user.
@P0lip
Copy link
Contributor

P0lip commented Feb 9, 2022

Hey!
I just wanted to let you know I noted this PR and it looks good to me.
I want to apply one change to duplicated-entry-in-enum rule, so as soon as I get that sorted out and merged, we'll be able to proceed with your PR.

@mkistler
Copy link
Contributor

mkistler commented Feb 9, 2022

This is a welcome change! I think many users will benefit from this. Great work @dpopp07!

@dpopp07
Copy link
Contributor Author

dpopp07 commented Feb 9, 2022

@P0lip Sounds great, thank you for the update!

@mkistler Thanks Mike! I hope all is well

Copy link
Contributor

@P0lip P0lip left a comment

Choose a reason for hiding this comment

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

Thank you!

@P0lip P0lip enabled auto-merge (squash) February 11, 2022 16:19
@P0lip P0lip merged commit b07cc7b into stoplightio:develop Feb 11, 2022
@dpopp07 dpopp07 deleted the dp/deduplication-function branch February 11, 2022 17:56
stoplight-bot pushed a commit that referenced this pull request Feb 14, 2022
# [@stoplight/spectral-core-v1.10.1](https://github.com/stoplightio/spectral/compare/@stoplight/spectral-core-v1.10.0...@stoplight/spectral-core-v1.10.1) (2022-02-14)

### Bug Fixes

* **core:** consider `message` when de-duplicating results ([#2052](#2052)) ([b07cc7b](b07cc7b))
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version @stoplight/spectral-core-v1.10.1 🎉

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.

Default "compute fingerprint" function removes unique validations
4 participants