BigDecimal Handling about fipp

[brandonbloom]

Thanks! Fixed by #74.

[brandonbloom]

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.

[matthewdowney]

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?

[brandonbloom]

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!

[matthewdowney]

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:

1. The way that I was originally thinking — extend 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.
2. Make a quick change to visit-number:

   (visit-number [this x]
     [:text #?(:clj (if (decimal? x) (str x "M") (str x))
               :cljs (str x))])

3. Modify IVisitor to
   • Add a (visit-decimal [this x]) method to IVisitor,
   • Update 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?),
   • Add a (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?

[brandonbloom]

(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

[matthewdowney]

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. 🤦

[matthewdowney]

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)?

[brandonbloom]

Some discussion of *print-dup* and the related *print-readably* can be found here: #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?

[matthewdowney]

Great, and that covers us for arbitrary precision integers ("N" suffix) as well. PR incoming.