Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ultra doesn't honor custom print-methods #8

Closed
ztellman opened this issue Jan 29, 2015 · 13 comments · Fixed by greglook/puget#18
Closed

ultra doesn't honor custom print-methods #8

ztellman opened this issue Jan 29, 2015 · 13 comments · Fixed by greglook/puget#18
Assignees
Milestone

Comments

@ztellman
Copy link

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
Copy link
Owner

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
Copy link

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
Copy link
Owner

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
Copy link
Contributor

👍 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
Copy link

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
Copy link
Owner

@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
Copy link
Contributor

@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
Copy link

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
Copy link
Owner

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 pushed a commit that referenced this issue Mar 1, 2015
This commit updates Puget and Whidbey to their latest versions, which
gives us access to the `:print-fallback` feature. This feature honors
objects' custom `print-methods`, and is a fix for
#8
@venantius
Copy link
Owner

@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
Copy link
Owner

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

@venantius venantius self-assigned this Mar 3, 2015
@venantius venantius modified the milestone: Ultra 0.3.0 Mar 3, 2015
@DizzeePascall
Copy link
Contributor

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

@venantius
Copy link
Owner

Marking as resolved unless @ztellman returns to say otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants