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

use HashWithIndifferentAccess, instead of symbolize_keys #386

Merged
merged 4 commits into from
Aug 19, 2021

Conversation

tilo
Copy link
Contributor

@tilo tilo commented Aug 18, 2021

use HashWithIndifferentAccess, instead of symbolize_keys

@tilo
Copy link
Contributor Author

tilo commented Aug 18, 2021

@chingor13, @gaorlov could you please review?

@gaorlov
Copy link
Collaborator

gaorlov commented Aug 18, 2021

This looks great! Can you please update the changelog md?
Thanks for taking the time!

@ryanbahniuk
Copy link

Thanks @tilo this will be a big help for us.

@tilo
Copy link
Contributor Author

tilo commented Aug 18, 2021

@gaorlov CHANGELOG is updated

@gaorlov
Copy link
Collaborator

gaorlov commented Aug 18, 2021

i meant to mention this earlier: what's the thinking behind adding pry to the gemspec? The approach so far has been to try to keep the gemspec footprint to a minimum unless absolutely necessary. Do you feel that it's necessary?
Thanks!

@tilo
Copy link
Contributor Author

tilo commented Aug 19, 2021

@gaorlov
I only added it as a development dependency, so that anyone who is working on a pull request can use pry locally on their machine to set a breakpoint and inspect data, while they are debugging any failing tests for the PR they make for json_api_client.

pry will not be included as a dependency for any down-stream project using json_api_client

@tilo
Copy link
Contributor Author

tilo commented Aug 19, 2021

@gaorlov do you think it's good to go, or do you want me to remove pry?

@gaorlov
Copy link
Collaborator

gaorlov commented Aug 19, 2021

The approach so far has been to keep the dependency footprint - both dev and release - to the minimum necessary to run and test. However, if you feel strongly that the addition of pry will benefit the gem I don't have any strong opposition to it.
Again: thank you for taking the time and your contribution!

@tilo
Copy link
Contributor Author

tilo commented Aug 19, 2021

I removed pry from this PR, and added a separate PR here: #385

@gaorlov gaorlov merged commit 7a6219e into JsonApiClient:master Aug 19, 2021
@gaorlov
Copy link
Collaborator

gaorlov commented Aug 19, 2021

1.19.0 is now live with your changes. Thanks for contributing!

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.

3 participants