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 options to EdnPacker to pass through to read-string. #90

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@ericnormand

ericnormand commented Nov 15, 2014

It should be possible to add custom tag handlers and other options for read-string.

@ptaoussanis

This comment has been minimized.

Show comment
Hide comment
@ptaoussanis

ptaoussanis Nov 15, 2014

Owner

Hi Eric, this patch unfortunately produces compilation warnings since cljs.reader/read-string is single-arity only. What options did you have in mind to use?

Owner

ptaoussanis commented Nov 15, 2014

Hi Eric, this patch unfortunately produces compilation warnings since cljs.reader/read-string is single-arity only. What options did you have in mind to use?

@ericnormand

This comment has been minimized.

Show comment
Hide comment
@ericnormand

ericnormand Nov 16, 2014

Upon further research into ClojureScript, I realize that I don't think what I wanted to do in the way I wanted it was possible.

I wanted to pass in {:readers {'my-project/custom-type #'my-project/read-custom-type}}, which is the way to get custom tagged literals in Clojure. Unfortunately, the interface is different in ClojureScript. You have to rebind a dynamic variable or something, so that's out for this use case. It would be way more complicated.

Sorry for the pull request.

ericnormand commented Nov 16, 2014

Upon further research into ClojureScript, I realize that I don't think what I wanted to do in the way I wanted it was possible.

I wanted to pass in {:readers {'my-project/custom-type #'my-project/read-custom-type}}, which is the way to get custom tagged literals in Clojure. Unfortunately, the interface is different in ClojureScript. You have to rebind a dynamic variable or something, so that's out for this use case. It would be way more complicated.

Sorry for the pull request.

@ptaoussanis

This comment has been minimized.

Show comment
Hide comment
@ptaoussanis

ptaoussanis Nov 16, 2014

Owner

Unfortunately, the interface is different in ClojureScript.

Yeah, that is a bit unfortunate. Haven't used custom tagged literals in Cljs myself, but seems like something like this should hopefully do the trick, despite being a bit clumsy.

Have just pushed to Clojars under [com.taoensso/sente "1.3.0-SNAPSHOT"] if you felt like giving it a try (it's completely untested).

Re-opening for now until we've confirmed this works and/or decide to revert it.

Cheers! :-)

Owner

ptaoussanis commented Nov 16, 2014

Unfortunately, the interface is different in ClojureScript.

Yeah, that is a bit unfortunate. Haven't used custom tagged literals in Cljs myself, but seems like something like this should hopefully do the trick, despite being a bit clumsy.

Have just pushed to Clojars under [com.taoensso/sente "1.3.0-SNAPSHOT"] if you felt like giving it a try (it's completely untested).

Re-opening for now until we've confirmed this works and/or decide to revert it.

Cheers! :-)

@ptaoussanis ptaoussanis reopened this Nov 16, 2014

ptaoussanis added a commit that referenced this pull request Jan 3, 2015

@ptaoussanis

This comment has been minimized.

Show comment
Hide comment
@ptaoussanis

ptaoussanis Jan 3, 2015

Owner

Closing for now. Have the change in the commit history; you're welcome to reopen if you still have interest in this going in. Cheers :-)

Owner

ptaoussanis commented Jan 3, 2015

Closing for now. Have the change in the commit history; you're welcome to reopen if you still have interest in this going in. Cheers :-)

@ptaoussanis ptaoussanis closed this Jan 3, 2015

@gphilipp

This comment has been minimized.

Show comment
Hide comment
@gphilipp

gphilipp Jul 19, 2015

Hi Peter & Eric, I'm using cli-time and have joda-time dates in the structures I want to pass back and forth between my Clojurescript app and my Clojure server. How can I do this either with edn or transit packer ? It seems like only this pull request would allow that (for edn at least).

gphilipp commented Jul 19, 2015

Hi Peter & Eric, I'm using cli-time and have joda-time dates in the structures I want to pass back and forth between my Clojurescript app and my Clojure server. How can I do this either with edn or transit packer ? It seems like only this pull request would allow that (for edn at least).

@ptaoussanis

This comment has been minimized.

Show comment
Hide comment
@ptaoussanis

ptaoussanis Jul 19, 2015

Owner

Hi Gilles,

PR welcome to reintroduce the previously-reverted commit if you can test it and confirm that it's working correctly?

Otherwise keep in mind that as a worst case you can always use pr-str to produce an edn string, send that over the wire, and manually read-string it back on the other end with whatever reader bindings you'd like like. Does that help?

Owner

ptaoussanis commented Jul 19, 2015

Hi Gilles,

PR welcome to reintroduce the previously-reverted commit if you can test it and confirm that it's working correctly?

Otherwise keep in mind that as a worst case you can always use pr-str to produce an edn string, send that over the wire, and manually read-string it back on the other end with whatever reader bindings you'd like like. Does that help?

@danielcompton

This comment has been minimized.

Show comment
Hide comment
@danielcompton

danielcompton Jul 19, 2015

Collaborator

@gphilipp here is some code that I'm using to add converters for Joda time dates into Transit and JSON. It's late here, I'll try and follow up with more details on it's use and whether it is another route to do what you're after.

(ns my-ns.utils
  (:require [cheshire.generate :as gen]
            [cognitect.transit :as transit]
            [clj-time.coerce :as coerce]
            [taoensso.sente :as sente]
            [taoensso.timbre :as timbre])
  (:import [org.joda.time DateTime ReadableInstant]
           [org.joda.time.format ISODateTimeFormat]))

(defn setup-encoders
  "Add converters to turn JODA Time objects into JSON dates"
  []
  (gen/add-encoder
    DateTime
    (fn [^DateTime dt ^com.fasterxml.jackson.core.json.WriterBasedJsonGenerator generator]
      (.writeString generator (.print (ISODateTimeFormat/dateTime) ^ReadableInstant dt)))))

;; TODO: Use native Java to avoid dep on clj-time?
(def joda-time-writer
  (transit/write-handler
    (constantly "m")
    (fn [v] (-> v coerce/to-date .getTime))
    (fn [v] (-> v coerce/to-date .getTime .toString))))
Collaborator

danielcompton commented Jul 19, 2015

@gphilipp here is some code that I'm using to add converters for Joda time dates into Transit and JSON. It's late here, I'll try and follow up with more details on it's use and whether it is another route to do what you're after.

(ns my-ns.utils
  (:require [cheshire.generate :as gen]
            [cognitect.transit :as transit]
            [clj-time.coerce :as coerce]
            [taoensso.sente :as sente]
            [taoensso.timbre :as timbre])
  (:import [org.joda.time DateTime ReadableInstant]
           [org.joda.time.format ISODateTimeFormat]))

(defn setup-encoders
  "Add converters to turn JODA Time objects into JSON dates"
  []
  (gen/add-encoder
    DateTime
    (fn [^DateTime dt ^com.fasterxml.jackson.core.json.WriterBasedJsonGenerator generator]
      (.writeString generator (.print (ISODateTimeFormat/dateTime) ^ReadableInstant dt)))))

;; TODO: Use native Java to avoid dep on clj-time?
(def joda-time-writer
  (transit/write-handler
    (constantly "m")
    (fn [v] (-> v coerce/to-date .getTime))
    (fn [v] (-> v coerce/to-date .getTime .toString))))
@danielcompton

This comment has been minimized.

Show comment
Hide comment
@danielcompton

danielcompton Sep 3, 2015

Collaborator

@gphilipp following up a few months later when I ran into this issue myself :)

(ns my-ns.app
  (:require [cognitect.transit :as transit]
            [taoensso.sente.packers.transit :as sente-transit])
  (:import [org.joda.time DateTime ReadableInstant]))

;; From http://increasinglyfunctional.com/2014/09/02/custom-transit-writers-clojure-joda-time/
(def joda-time-writer
  (transit/write-handler
    (constantly "m")
    (fn [v] (-> ^ReadableInstant v .getMillis))
    (fn [v] (-> ^ReadableInstant v .getMillis .toString))))

(def custom-transit-writers {DateTime joda-time-writer})

(def packer (sente-transit/->TransitPacker :json {:handlers utils/custom-transit-writers} {}))
Collaborator

danielcompton commented Sep 3, 2015

@gphilipp following up a few months later when I ran into this issue myself :)

(ns my-ns.app
  (:require [cognitect.transit :as transit]
            [taoensso.sente.packers.transit :as sente-transit])
  (:import [org.joda.time DateTime ReadableInstant]))

;; From http://increasinglyfunctional.com/2014/09/02/custom-transit-writers-clojure-joda-time/
(def joda-time-writer
  (transit/write-handler
    (constantly "m")
    (fn [v] (-> ^ReadableInstant v .getMillis))
    (fn [v] (-> ^ReadableInstant v .getMillis .toString))))

(def custom-transit-writers {DateTime joda-time-writer})

(def packer (sente-transit/->TransitPacker :json {:handlers utils/custom-transit-writers} {}))
@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 6, 2015

Collaborator

A little contribution to the discussion, especially to the attention of boot users. :-)

I was trying to achieve exactly the same: passing custom edn tags for Joda. I'm not using transit. Not that I have anything against transit, but I'm in full control of the data passing through my system. Since my system is 100% Clojure, what flows through the various pieces (server, client) is edn. Blissful simplicity (I know transit is faster, but it's meaningless in my use case).

I've defined custom reader tags like so: https://gist.github.com/Deraen/eb3f650c472fb1abe970#file-edn-cljx

With sente, I have not defined any flexipacker, so if my understanding is correct the default is used, which happens to be :edn.

So far so good.

I was debugging however an instance of

clojure.lang.ExceptionInfo: No reader function for tag DateTime
    data: {:type :reader-exception}

The problem was that data-readers was not populated. I tried both with writing data_readers.clj and via rebinding data-readers. Long story short, the issue was with boot: boot-clj/boot#47

Collaborator

danielsz commented Dec 6, 2015

A little contribution to the discussion, especially to the attention of boot users. :-)

I was trying to achieve exactly the same: passing custom edn tags for Joda. I'm not using transit. Not that I have anything against transit, but I'm in full control of the data passing through my system. Since my system is 100% Clojure, what flows through the various pieces (server, client) is edn. Blissful simplicity (I know transit is faster, but it's meaningless in my use case).

I've defined custom reader tags like so: https://gist.github.com/Deraen/eb3f650c472fb1abe970#file-edn-cljx

With sente, I have not defined any flexipacker, so if my understanding is correct the default is used, which happens to be :edn.

So far so good.

I was debugging however an instance of

clojure.lang.ExceptionInfo: No reader function for tag DateTime
    data: {:type :reader-exception}

The problem was that data-readers was not populated. I tried both with writing data_readers.clj and via rebinding data-readers. Long story short, the issue was with boot: boot-clj/boot#47

@ptaoussanis

This comment has been minimized.

Show comment
Hide comment
@ptaoussanis

ptaoussanis Dec 6, 2015

Owner

Thanks Daniel, that'll be super helpful to have here as a reference for others!

Owner

ptaoussanis commented Dec 6, 2015

Thanks Daniel, that'll be super helpful to have here as a reference for others!

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 6, 2015

Collaborator

Thank you, @ptaoussanis!

And the good new is that the issue is fixed in system as of 0.2.1-SNAPSHOT.

https://github.com/danielsz/system

Collaborator

danielsz commented Dec 6, 2015

Thank you, @ptaoussanis!

And the good new is that the issue is fixed in system as of 0.2.1-SNAPSHOT.

https://github.com/danielsz/system

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 6, 2015

Collaborator

It was a bit too soon to claim victory.

I'm sending the following payload from the client.

15-Dec-06 09:34:28 daniel-MacBookAir DEBUG [taoensso.sente] - Bad package: [[:twitter-fu/api {:update-preferences {:shop-name "test", :accounts {:twitter "bellybag1" :legacy {:end #DateTime "20151205T000000.000Z", :provider "shopify"}}, :total-items 5}}] "3cc733"] (clojure.lang.ExceptionInfo: No reader function for tag DateTime {:type :reader-exception})

Sente is unaware of the custom tag which has been registered here:

(cljs.reader/register-tag-parser! "DateTime" reader-str->datetime)

Can you please advise?

Collaborator

danielsz commented Dec 6, 2015

It was a bit too soon to claim victory.

I'm sending the following payload from the client.

15-Dec-06 09:34:28 daniel-MacBookAir DEBUG [taoensso.sente] - Bad package: [[:twitter-fu/api {:update-preferences {:shop-name "test", :accounts {:twitter "bellybag1" :legacy {:end #DateTime "20151205T000000.000Z", :provider "shopify"}}, :total-items 5}}] "3cc733"] (clojure.lang.ExceptionInfo: No reader function for tag DateTime {:type :reader-exception})

Sente is unaware of the custom tag which has been registered here:

(cljs.reader/register-tag-parser! "DateTime" reader-str->datetime)

Can you please advise?

@ptaoussanis

This comment has been minimized.

Show comment
Hide comment
@ptaoussanis

ptaoussanis Dec 6, 2015

Owner

Hi Daniel,

Please make sure that you're using [com.taoensso/encore "2.26.1"] or newer; some work went in there recently to try pave over issues with ClojureScript's reader tag support. The next Sente release will auto-bump this dependency but it's not included with Sente 1.7.0-RC1 yet.

If that's no help, could you possibly try confirm whether the issue still exists when not using Boot?

Thanks!

Owner

ptaoussanis commented Dec 6, 2015

Hi Daniel,

Please make sure that you're using [com.taoensso/encore "2.26.1"] or newer; some work went in there recently to try pave over issues with ClojureScript's reader tag support. The next Sente release will auto-bump this dependency but it's not included with Sente 1.7.0-RC1 yet.

If that's no help, could you possibly try confirm whether the issue still exists when not using Boot?

Thanks!

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 6, 2015

Collaborator

Hi Peter,

Yes, that was it. No such problem with [com.taoensso/encore "2.26.1"]. Thank you!

Am I allowed to rant?

Having old versions of the two auxiliary libraries encore and timbre in my classpath is something of a fixture. I remember old versions of Timbre giving me trouble (although that may have been in conjunction with Carmine). Besides being a potential source of problems, it is a difficult one to debug from a user's perspective. I never think of checking the latest versions, because these are no part of my project dependencies, and my expectation is that the latest Sente is really the latest (including dependencies). I see Sente ships with [com.taoensso/encore "2.18.0"]. Do you expect users to track versions of the project's dependencies themselves? I mean, is there a reason for this state of affairs?

End of rant. :-)

Collaborator

danielsz commented Dec 6, 2015

Hi Peter,

Yes, that was it. No such problem with [com.taoensso/encore "2.26.1"]. Thank you!

Am I allowed to rant?

Having old versions of the two auxiliary libraries encore and timbre in my classpath is something of a fixture. I remember old versions of Timbre giving me trouble (although that may have been in conjunction with Carmine). Besides being a potential source of problems, it is a difficult one to debug from a user's perspective. I never think of checking the latest versions, because these are no part of my project dependencies, and my expectation is that the latest Sente is really the latest (including dependencies). I see Sente ships with [com.taoensso/encore "2.18.0"]. Do you expect users to track versions of the project's dependencies themselves? I mean, is there a reason for this state of affairs?

End of rant. :-)

@ptaoussanis

This comment has been minimized.

Show comment
Hide comment
@ptaoussanis

ptaoussanis Dec 7, 2015

Owner

Yes, that was it. No such problem with [com.taoensso/encore "2.26.1"]. Thank you!

No problem. Will try cut a new Sente release with the fix in the next few days, just have my hands a bit full atm.

Having old versions of the two auxiliary libraries encore and timbre in my classpath is something of a fixture.

Fixture?

I never think of checking the latest versions, because these are no part of my project dependencies, and my expectation is that the latest Sente is really the latest (including dependencies).

You don't need to think of checking the latest versions (that'd be documented otherwise), your expectation is correct that libs should pull in the necessary dependencies.

In this case: you brought a Sente issue to my attention for the first time. I suspected that maybe a new version of Encore would help. You just confirmed that. So my intention is to bump the Encore dependency for Sente's next overdue release, fixing the issue. This way you get the fix sooner than if you'd have had to wait for the next Sente release so not sure what state of affairs here warrants ranting?

Does that make sense / seem reasonable?

Edit: just to clarify a possible source of confusion - I wasn't intending to suggest that you're running into this issue because you failed to check for the latest dependencies. You shouldn't need to. Rather: was asking you to try with the latest dependencies to see if that might solve the problem.

Owner

ptaoussanis commented Dec 7, 2015

Yes, that was it. No such problem with [com.taoensso/encore "2.26.1"]. Thank you!

No problem. Will try cut a new Sente release with the fix in the next few days, just have my hands a bit full atm.

Having old versions of the two auxiliary libraries encore and timbre in my classpath is something of a fixture.

Fixture?

I never think of checking the latest versions, because these are no part of my project dependencies, and my expectation is that the latest Sente is really the latest (including dependencies).

You don't need to think of checking the latest versions (that'd be documented otherwise), your expectation is correct that libs should pull in the necessary dependencies.

In this case: you brought a Sente issue to my attention for the first time. I suspected that maybe a new version of Encore would help. You just confirmed that. So my intention is to bump the Encore dependency for Sente's next overdue release, fixing the issue. This way you get the fix sooner than if you'd have had to wait for the next Sente release so not sure what state of affairs here warrants ranting?

Does that make sense / seem reasonable?

Edit: just to clarify a possible source of confusion - I wasn't intending to suggest that you're running into this issue because you failed to check for the latest dependencies. You shouldn't need to. Rather: was asking you to try with the latest dependencies to see if that might solve the problem.

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 7, 2015

Collaborator

Yes, absolutely. I'm sorry for the tone of the rant.
If I had any trouble with your otherwise excellent libraries, it was with the auxiliary ones (timbre and encore). It is my responsibility to report it when it happens, not as a result of past frustrations.

Collaborator

danielsz commented Dec 7, 2015

Yes, absolutely. I'm sorry for the tone of the rant.
If I had any trouble with your otherwise excellent libraries, it was with the auxiliary ones (timbre and encore). It is my responsibility to report it when it happens, not as a result of past frustrations.

@ptaoussanis

This comment has been minimized.

Show comment
Hide comment
@ptaoussanis

ptaoussanis Dec 7, 2015

Owner

No problem, happy you reported this :-)

Owner

ptaoussanis commented Dec 7, 2015

No problem, happy you reported this :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment