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

Convert have_id matcher argument to string. #31

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wildfiler
Copy link

What is the current behavior?

According to docs this two lines should be equivalent:

expect(document['data']).to have_id(12)
expect(document['data']).to have_id('12')

But the first line will fail if the id value in the hash is a string, and the second line will fail if the id value is another type than a string.

What is the new behavior?

Convert expected value and actual value to a string in the have_id matcher.

Checklist

Please make sure the following requirements are complete:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes /
    features)
  • All automated checks pass (CI/CD)

@stas
Copy link
Collaborator

stas commented Jan 17, 2022

@wildfiler I'm afraid your JSONAPI document is invalid if it is using integers:
https://jsonapi.org/format/#document-resource-object-identification

Every resource object MUST contain an id member and a type member. The values of the id and type members MUST be strings.

I think the PR makes sense, as long as we convert just the expected value into string. @wildfiler would you be kind to update your PR please?

@wildfiler
Copy link
Author

@stas Yes, I agree and I did it in the first iteration, but after that, I saw this issue: #29 and related PR: #30

If you think this should not be a problem with id then I'll change it :)

@stas
Copy link
Collaborator

stas commented Jan 17, 2022

Nah, all good, please feel free to update it and I'll get it merged and released.

Thank you! 🙇

@wildfiler wildfiler force-pushed the fix-have-id-matcher branch from 8486008 to 27bbefd Compare January 17, 2022 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants