Skip to content

have_type comparison consistency improvements #25

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

Closed

Conversation

kjuri
Copy link

@kjuri kjuri commented Aug 28, 2020

What is the current behavior?

Currently, we're assuming that actual type is always a string, which causes test failures whenever type is a symbol.
In my case it is caused by those lines of JSONAPI::Serializable::Resource:

        @_type = if (b = self.class.type_block)
                   instance_eval(&b).to_sym
                 else
                   self.class.type_val || :unknown
                 end

It always transforms type to a symbol so the tests fail as have_type compares it to a string.

What is the new behavior?

The actual type is transformed into a string for a comparison consistency.

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)

@kjuri kjuri changed the title Have type consistency improvements have_type comparison consistency improvements Aug 28, 2020
@stas
Copy link
Collaborator

stas commented Aug 28, 2020

@kjuri I'd be happy to merge it, but following the JSONAPI Specs a type should always be a string and it's optional in some cases.

I believe the implementation you are referring to needs to be fixed outside of this library.

Let me know if this makes sense. Thank you though!!! 🙇

@kjuri
Copy link
Author

kjuri commented Aug 31, 2020

@stas Thanks for clarifying, maybe PR should be done to JSONAPI::Serializable::Resource then :)

@kjuri kjuri closed this Aug 31, 2020
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