search

inaka / Dayron

A repository `similar` to Ecto.Repo that maps to an underlying http client, sending requests to an external rest api instead of a database
http://inaka.net/blog/2016/05/24/introducing-dayron/
Apache License 2.0
159 stars 20 forks source link

Use Poison atom keys #58

Open jwarlander opened 7 years ago

jwarlander commented 7 years ago

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 commented 7 years ago

Coverage Status

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

cloud8421 commented 7 years ago

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

jwarlander commented 7 years ago

@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 commented 7 years ago

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