Skip to content

Use Poison atom keys#58

Open
jwarlander wants to merge 2 commits intoinaka:masterfrom
jwarlander:use-poison-atom-keys
Open

Use Poison atom keys#58
jwarlander wants to merge 2 commits intoinaka:masterfrom
jwarlander:use-poison-atom-keys

Conversation

@jwarlander
Copy link
Copy Markdown

Instead of manually converting JSON keys to atoms in Dayron.HTTPoisonAdapter, eliminate a dependency and some code by using the keys: :atoms option for Poison when decoding.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 076cdcc on jwarlander:use-poison-atom-keys into 6d06d94 on inaka:master.

@cloud8421
Copy link
Copy Markdown

cloud8421 commented Oct 12, 2016

Great to drop a dependency, but this triggered a question around dynamic atoms for me, I'll raise a n issue.

@jwarlander
Copy link
Copy Markdown
Author

@cloud8421, it should behave the same as the code it replaces (https://github.com/devinus/poison/blob/master/lib/poison/parser.ex#L113-L114) - eg. yes, it will create potentially infinite atoms until the VM crashes.

As mentioned in #57, this can't really be changed until there's a new major release, as it would potentially break existing use cases, so I just went for eliminating a dependency for now.

@cloud8421
Copy link
Copy Markdown

@jwarlander thanks, I raised a separate issue at #59 about it. Yes, it definitely needs a deprecation/upgrade path if it gets changed.

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