jimpil / duratom

A durable atom type for Clojure
Eclipse Public License 1.0
213 stars 14 forks source link

Lost meta information / why not *print-dup* in pr-str-fully? #10

Closed razum2um closed 5 years ago

razum2um commented 5 years ago

I'm taking about this place https://github.com/jimpil/duratom/blob/master/src/clojure/duratom/utils.clj#L25

Given https://github.com/clojure/clojure/blob/ee3553362de9bc3bfd18d4b0b3381e3483c2a34c/src/clj/clojure/core.clj#L3672L3674 and according to https://clojuredocs.org/clojure.core/*print-dup* I understand that print-dup is preferred way to serialise anything in order to restore it later, and print-method is used for console printing, just human representation etc..

Meaningful differences:

user=> ((juxt identity (comp meta read-string)) (binding [*print-dup* true] (pr-str (with-meta {:data 1} {:meta 1}))))
["^#=(clojure.lang.PersistentArrayMap/create {:meta 1}) #=(clojure.lang.PersistentArrayMap/create {:data 1})" {:meta 1}]
user=> ((juxt identity (comp meta read-string)) (binding [*print-dup* false] (pr-str (with-meta {:data 1} {:meta 1}))))
["{:data 1}" nil]

So duratom loses meta info by default. On the other hand print-dup string should be read-string not a edn/read-string.

I suspect introducing this will break compatibility with written files, and I understand that :rw gives an easy option to configure it, just wonder why such default? Only because of sticking with edn?

Maybe warn users about this as well / add example in README how to serialise via print-dup/restore with read-string?

jimpil commented 5 years ago

Binding *print-dup* to true, produces those literals, as you've demonstrated. In order to read those back, you also need *read-eval* bound to true, which I don't think is a reasonable default. However, your question made me realise that there is a related issue here. Apart from the metadata being lost, duratom also looses types (by default). For example, a sorted-map will be serialised as a regular map. That definitely needs to be explained/warned, and an example should be provided in the README. I will do my best to update it by end-of-week. Many thanks for bringing this to my attention :+1:

jimpil commented 5 years ago

Hi there,

I've added support for sorted-map, sorted-set & clojure.lang.PersistentQueue. These are great candidates for use with atoms, and so starting with the next release these will be printed/read correctly (without losing the actual type).

On your metadata issue/concern/query, I've added some baked-in (magical) support for objects with metadata, but I would encourage you to read the new section(s) in the README, especially the one about metadata. In short, I feel the best way to handle this is outside of duratom.

If you have any further questions, and/or feedback, these are welcome. :smile:

Kind regards

harold commented 5 years ago

Have I mentioned I love this library?

This is an interesting thread, reminder that nippy support avoids these problems and brings many other benefits as well (smaller size on disk, faster serialization and deserialization). We have more data in nippy backed duratoms than you think (even if you think we have a lot of data in nippy backed duratoms), and couldn't be happier.

eval is scary - agreed not a good default.

Keep up the great work.

jimpil commented 5 years ago

Hi @harold , thanks for your kind words - they made my day :).

@razum2um you can safely ignore my comment from yesterday - I wasn't thinking clearly... duratom now natively supports metadata for the default EDN readers/writers. See the updated README section. I think i'll stick with this approach, because as far as I can see it shouldn't break existing programs - even though the default behaviour has changed. Moreover, the previous behaviour can be trivially reinstated.

As always feedback is most welcome ;)

jimpil commented 5 years ago

0.4.2 has been released - closing this.

@razum2um thanks for reporting this, and forcing me to think about non-edn collections - i consider this update a rather important one.

@harold I would appreciate if you tried out this new version and see if anything breaks. Huge thanks... :)

harold commented 5 years ago

@jimpil - if I understand correctly, this change won't affect us. Our libraries are surprisingly well factored and we call duratom/duratom in exactly one place and it looks like this:

(duratom/duratom :local-file
         :file-path file-path
         :init {}
         :rw {:read nippy/thaw-from-file
                      :write nippy/freeze-to-file})

That is, we never use the default :rw. :)

If we do ever upgrade and encounter trouble, I'll let you know.

Have a great day.

jimpil commented 5 years ago

@harold correct, given this setup you shouldn't be affected at all :)

harold commented 5 years ago

:bowing_man: