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

add symbolize keys option to deserializer #70

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lHydra
Copy link

@lHydra lHydra commented Oct 28, 2021

What is the current behavior?

At the moment, the call to the jsonapi_deserialize method returns hash with string keys

What is the new behavior?

Method will provide new option symbolize_key which user can set to true and the returned hash will be with symbolized keys

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
Owner

stas commented Nov 8, 2021

Hey hey, apologies for being late with the reviews here!

I have a couple of concerns merging this @lHydra
My main ones are that symbolize_keys is part of the ActiveSupport and not generally available to folks not running rails. And you probably want deep_symbolize_keys instead, as the dict might result nested.

Another bigger concern, is that I'd prefer to leave it up to you (the end-user) to decide how to handle the parsed payload.

So if we have to change the api from (which is how it's working now):

jsonapi_deserialize(params, only: [:email]).deep_symbolize_keys

into

jsonapi_deserialize(params, symbolize_keys: true, only: [:email])

I'd prefer to keep things the way these are. I hope this makes sense.

Thank you very much for the PR though. I'm happy to discuss such concerns in the issues in future, just to avoid extra work on the PR/code. 🙇 🤗

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