Closed matthewdowney closed 4 years ago
I think a patch would be welcome here, but not exactly sure what it should do. Fipp always "ednizes" data, but BigDecimal
notation (the trailing M
) is not valid edn as far as I know. Seems like inventing our own tagged-literal for it would be weird. I'm open to suggestions.
I thought that might have been it too, but I looked at the spec and it does mention, under floating point numbers:
In addition, a floating-point number may have the suffix M to indicate that exact precision is desired.
If I'm reading that right / what I'm asking for is desired behavior, I'd be happy to write a patch.
Would it be as simple as modifying ednize.clj (since cljs doesn't have a BigDecimal equivalent) and extending the IEdn
protocol for decimals?
but I looked at the spec
Ah, that's great then!
Would it be as simple as modifying ednize.clj (since cljs doesn't have a BigDecimal equivalent) and extending the IEdn protocol for decimals?
Yup, that should do it. PR welcome. Please update the tests as well. Thanks!
Great. After reading the code and sitting down to figure out an implementation, I think the way that I proposed above is actually the worst of three potential approaches.
Also as a side note, I really like the way you've designed this library! Very pleasant to read through. :)
The three approaches I'm considering are:
IEdn
and IOverride
to apply to java.math.BigDecimal
. The implementation of -edn
would have to return (symbol (str x "M"))
. Otherwise visit
gets called with a string value, which gets surrounded with quotes.visit-number
:
(visit-number [this x]
[:text #?(:clj (if (decimal? x) (str x "M") (str x))
:cljs (str x))])
IVisitor
to
(visit-decimal [this x])
method to IVisitor
,visit*
to check for a utils/decimal?
(which I'd implement like utils/char?
) before checking for number?
(similar to how the ordering inside cond
is careful to check for record?
before map?
),(visit-decimal [this x])
implementation to EdnPrinter
.(1) feels a bit awkward, (2) is nice and quick, but (3) feels more in line with how the only other Clojure-only EDN construct (char) is handled. But (2) is less intrusive.
What do you think?
(1) is definitely too ugly to be right :) (2) seems promising. BigDecimal does satisfy Number, after all. (3) would be perfectly reasonable, but all the other cases are disjoint types except for the overlap of strings/chars in ClojureScript.
I was about to reply "let's go with (2)", but then I decided to try this:
user=> (pr-str 5M)
"5M"
Boom!
Unless you can think of another number type that pr-str
will print in an unacceptable way, let's go with that :) Probably don't even need to bother with a test
Hahaha thank you, that's way better. I thought that I tried that, but I used pr
instead of pr-str
and ran the tests without even thinking about it. :facepalm:
Huh it breaks an assertion in fipp.edn-test/pprint-edn:
(testing "Not affected by *print-dup* binding"
(is (= (with-out-str (binding [*print-dup* true]
(pprint [(Integer. 1) (Long. 2)])))
(with-out-str (pprint [(Integer. 1) (Long. 2)]))
"[1 2]\n")))
; FAIL in (pprint-edn) (edn_test.cljc:179)
; Not affected by *print-dup* binding
; expected: (= (with-out-str (binding [*print-dup* true] (pprint [(Integer. 1) (Long. 2)]))) (with-out-str (pprint [(Integer. 1) (Long. 2)])) "[1 2]\n")
; actual: (not (= "[#=(java.lang.Integer. \"1\") 2]\n" "[1 2]\n" "[1 2]\n"))
For context:
(binding [*print-dup* true]
(let [x (Integer. 1)]
{:pr-str (pr-str x)
:str (str x)
:core.pprint (with-out-str (pprint x))}))
;=> {:pr-str "#=(java.lang.Integer. \"1\")",
; :str "1",
; :core.pprint "#=(java.lang.Integer. \"1\")\n"}
Back to (2)?
Some discussion of *print-dup*
and the related *print-readably*
can be found here: https://github.com/brandonbloom/fipp/pull/56
For strings and characters, we call pr-str, but bind *print-readably*
to true
. Can we do the same for binding *print-dup*
to false
when stringifying numbers?
Great, and that covers us for arbitrary precision integers ("N" suffix) as well. PR incoming.
Thanks! Fixed by #74.
Hi there,
Thanks for this great library! I'm finding that
BigDecimal
s don't survive a print/read cycle: