venantius / ultra

A Leiningen plugin for a superior development environment
Eclipse Public License 1.0
1.24k stars 35 forks source link

ultra doesn't honor custom `print-methods` #8

Closed ztellman closed 9 years ago

ztellman commented 9 years ago

With Ultra, using lein repl on Manifold:

user=> (require '[manifold.deferred :as d])
nil
user=> (d/deferred)
#<manifold.deferred.Deferred@674a6631 pending>

w/o ultra:

user=> (require '[manifold.deferred :as d])
nil
user=> (d/deferred)
<< … >>

I didn't take any time to dig into this, I would assume it's relatively straightforward to fix, though. Otherwise, very cool project.

venantius commented 9 years ago

While I'm completely unfamiliar with Manifold, my guess is that this is an underlying issue with Whidbey, which injects some nREPL middleware in that is likely overriding something. Might take me a while to ferret this down.

greglook commented 9 years ago

This is because whidbey (which uses puget under the hood) has no integration with the built in print-method or print-dup multimethods. You can think of the built-ins as converting values into printed strings - Puget can't really know a-priori how to colorize those strings without doing parsing and syntax highlighting, which is way out of scope.

Puget's format-doc multimethod instead turns values into print documents, which are then serialized into text by fipp. If you want to add custom printing for values, you should define a new method for puget.printer/format-doc which outputs the appropriate data structure.

venantius commented 9 years ago

I think it's probably undesirable to force anybody who uses Ultra to write custom print-methods for their respective data structures. I'll try to think of some way of intelligently checking to see if Puget knows how to represent the data in question, and if not to defer to the data structure's native print methods.

DizzeePascall commented 9 years ago

:+1: I ran into this issue today

@venantius I don't think the behaviour you've outlined above is the desired behaviour (at least in my case). For example, my company has a print-method for URIs so that they print as e.g. #org-name/uri "www.google.com" but Puget overrides this as #uri "www.google.com". Even with your suggested changes this would produce different serialisation on machines with Ultra to machines without it.

I'd prefer custom print-method methods to take preference over Puget

greglook commented 9 years ago

One option would be to add an option to Puget like :print-fallback which could select whether to use its built-in unknown formatter or Clojure's print-method.

venantius commented 9 years ago

@greglook Is print-method defined for all known classes such that checking to see if they have an implementation is a waste of time? I'm wondering if it might make sense (and/or be possible) to simply do an equality check on a given class' :print-method against some default or set of default :print-methods and if that fails to use the custom :print-method.

In the short term something like what you're describing makes sense as an escape hatch for 90% of Ultra's use cases, I'm just trying to think about what the sensible longer term solution might look like.

DizzeePascall commented 9 years ago

@venantius print-method has a default implementation. Is it possible to check whether there's a custom print-method before using Puget's alternative printing implementations?

TBH, I'd prefer the option @greglook suggests of having the option to disable Puget's formatter. It'd be simple and is the behaviour that I'd expect when using a general purpose dev environment like Ultra

greglook commented 9 years ago

If you look at the relevant code in Puget, you can see that the default dispatch for format-doc calls the single-argument arity of unknown-document. This uses str to render the value inside the <class@id repr> format.

A really simple change would be to use pr-str instead, so we would get any custom print-method output included in the document at least. I think the best alternate option is to add the :print-fallback config as above, which would change the default dispatch to just use pr-str and omit the other markup around the value.

venantius commented 9 years ago

After starting work on a Puget PR, I've come to realize that Manifold is something of a special case - it satisfies clojure.lang.IPending, which has a special format-doc method.

Since the Manifold deferred isn't realized, this ends up calling color-doc and going a rather different execution path, which ends up using the 3-arity version of unknown-doc.

Personally, my preference would be to default to just using the pr-str of unrealized clojure.lang.IPending values rather than explicitly evaluating their class names and printing those, but I'm open to more elegant solutions that I haven't considered.

venantius commented 9 years ago

@ztellman I believe this issue should be resolved with Ultra 0.3.0. If you're feeling so inclined, I'd recommend updating and giving it a try.

venantius commented 9 years ago

Better make that 0.3.2, given my release problems today :P

DizzeePascall commented 9 years ago

Works great for me, thanks for the fix. Now I can use Ultra at work again :+1:

venantius commented 9 years ago

Marking as resolved unless @ztellman returns to say otherwise.