taoensso / faraday

Amazon DynamoDB client for Clojure
https://www.taoensso.com/faraday
Eclipse Public License 1.0
238 stars 84 forks source link

Use extensible protocol for serialisation #95

Closed garethmdavies closed 7 years ago

garethmdavies commented 8 years ago

We are trying to serialise dates into our dynamodb table and plan to use prismatic schema to deserialise and coerce the data out again. Using protocols allows us to achieve this by implementing a date to string conversion locally without affecting the faraday library. An example of the date serialisation that we wish to use has been included in the test.

ptaoussanis commented 8 years ago

Hi there, thanks for the PR!

Should be okay in principle but will need some time to take a proper look, will get back to you. Cheers :-)

garethmdavies commented 8 years ago

Great thanks.

garethmdavies commented 7 years ago

Did you get chance to look at this?

ricardojmendez commented 7 years ago

I just cloned it myself, but it looks like that new test is failing.

failure in (main.clj:367) : taoensso.faraday.tests.main
(expect
 (assoc t :date "Thu Jan 01 01:00:00 GMT 1970")
 (update-t {:update-map {:date [:put (java.util.Date. (long 0))]}}))

           expected: {:boolF false, :num 1, :date "Thu Jan 01 01:00:00 GMT 1970", :vec ["a" 1 false nil], :strset #{"a" "b" "c"}, :numset #{4 13 6 12}, :boolT true, :string "string", :null nil, :id 14, :map {:k1 "v1", :k2 "v2", :k3 "v3"}}
                was: {:boolF false, :num 1N, :date "Thu Jan 01 02:00:00 EET 1970", :vec ["a" 1N false nil], :strset #{"a" "b" "c"}, :numset #{4N 13N 6N 12N}, :boolT true, :string "string", :null nil, :id 14N, :map {:k1 "v1", :k2 "v2", :k3 "v3"}}

           in expected, not actual: {:date "Thu Jan 01 01:00:00 GMT 1970"}
           in actual, not expected: {:date "Thu Jan 01 02:00:00 EET 1970"}
ricardojmendez commented 7 years ago

There also seems to be a conflict likely because of the recent dependency updates. Could you please re-base against the latest, re-run the tests and re-submit?

Thanks!

garethmdavies commented 7 years ago

Rebased and fixed the test.

ricardojmendez commented 7 years ago

All tests pass both locally and when running them on my Gitlab CI. Merging.

ptaoussanis commented 7 years ago

@taffowl Sorry for the long delay in handling this, thanks again for the PR.

@ricardojmendez Great, thanks Ricardo! Please note that you'll want to rebase against master since I rewrote history on this last commit.

About that: could I possibly request that any future PRs you commit use the same repo style, e.g.: [#95] Short description (@submitter).

This style only takes a moment longer when accepting a commit, but can save significant effort when cutting new releases, and can be helpful when scanning history, etc.

Much appreciated, cheers! :-)

ricardojmendez commented 7 years ago

@ptaoussanis Got it, will ping you before the next one just in case.

ptaoussanis commented 7 years ago

Oh, no pinging necessary - please feel free to do as you've been doing. It's a huge help, thanks! :-)