brandonbloom / fipp

Fast Idiomatic Pretty Printer for Clojure
525 stars 44 forks source link

Some map-like objects are printed incorrectly #37

Closed Malabarba closed 5 years ago

Malabarba commented 8 years ago

See https://github.com/clojure-emacs/cider/issues/1505. At least one example are datomic entities one gets by doing something like

(datomic.api/entity db [:db/ident :some/attr])

Normally they would print like this:

{:db/id 96}

With fipp they are printed like this:

#object[datomic.query.EntityMap "0x5123dcf1" "{:db/id 96}"]
brandonbloom commented 8 years ago

Presumably this is because Datomic entities do not implement IPersistentMap and therefore map? returns false.

What does (-> entity class supers) give you?

Malabarba commented 8 years ago

The class is datomic.query.EntityMap. The supers returns:

#{datomic.query.EMapImpl java.lang.Object clojure.lang.IPersistentCollection clojure.lang.Seqable clojure.lang.ILookup clojure.lang.IType datomic.Entity clojure.lang.Associative}

I believe datomic.query.EntityMap extends print-method. Is it possible that's not getting used?

brandonbloom commented 8 years ago

Fipp's built in Edn printer intentionally does not respect print-method for two reasons:

  1. Such objects would not be pretty-printed.
  2. Fipp would be unable to guarantee that the output would be readable.

To elaborate on the second point: As of the introduction of tagged literals, Fipp output promises to be valid Edn. That is, no #< ... > forms are emitted. Anything that might be unreadable will instead be emitted as a tagged literal with a quoted pr-str value.

There's a few possible solutions here to accommodate Datomic:

  1. Use the Fipp engine, but a custom Edn printer. This is the approach that @cursive-ide chose, as they output IntelliJ display objects instead of text. Colin can probably provide details if you're interested.
  2. Extend the fipp.edenize.IEdn protocol. This protocol has one method: -edn which converts a type in to a standard Clojure type for later printing. Note: I reserve the right to change that protocol! It's unlikely, but possible.
  3. Propose additional options or extension points for Fipp.

Do any of these work for you?

Malabarba commented 8 years ago

Option 2 sounds like the easiest of those 3, but there's also option 4:

4: Since fipp does this by design and doesn't consider it a bug then we close both issues as not-a-bug. :-)

That's what I would do, but maybe @bbatsov or @cichli have a different opinion.

bbatsov commented 8 years ago

Option 4 would mean dropping support for fipp from CIDER. This is fine by me, but others might disagree. I simply don't want to deal with users complaining about behaviours they deem unexpected.

Malabarba commented 8 years ago

Option 4 would mean dropping support for fipp from CIDER.

Or just not making it the default value of cider-pprint-fn.

bbatsov commented 8 years ago

I don't think it's the default even now (we changed it when people started complaining). But there's no point in supporting it, if we know some people will run into such issues. Pretty sure @cichli wasn't aware of those problems when he elected to add it to cider and make it the default.

brandonbloom commented 8 years ago

It's possible to endow Fipp with a "best effort" pretty printing mode, where it falls back to print-method where appropriate. However, you'll probably get the followup complaint: "How come Datomic entities are not pretty printed?" Thoughts on that?

brandonbloom commented 8 years ago

And on the topic of extending a protocol: I could entertain making that an official public protocol, but I don't want to be in the business of extending that protocol to all the popular custom types out there. I doubt you want to be in that business either. However, entertaining the thought for a minute: What other types have people complained about?

bbatsov commented 7 years ago

I don't recall anything besides the Datomic complaints.

brandonbloom commented 5 years ago

I'm going to close this, since the current behavior is expected and -edn is pretty stable, so safe to override if you want to add specific improvements for datomic in your project. Thanks for the report.