search

JuliaWeb / Hyperscript.jl

Hyperscript: A lightweight DOM representation for Julia
Other
101 stars 11 forks source link

Use `show` instead of `repr`. #29

Closed MichaelHatherly closed 3 years ago

MichaelHatherly commented 3 years ago

Allows for passing IOContext through to underlying show methods for arbitary types embedded in the dom. This doesn't appear to cause any tests to fail, but given the direct reference to repr in the comment above this method I'm not 100% whether the fix here does actually cause issues, though I'd assume it wouldn't...

Some context for the issue: I've got types embedded in hyperscript dom elements where their show methods depend on information carried through in an IOContext, using repr here discards the context.

yurivish commented 3 years ago

Hi @MichaelHatherly, thanks for the PR! This seems like a reasonable change and I can't think of any cases I was specifically targeting with my use of repr. Can you think of any significant cases where the two differ?

MichaelHatherly commented 3 years ago

There shouldn't be a difference. The implementation uses show underneath (https://github.com/JuliaLang/julia/blob/76106100559e6dc61769cdbd3d53836e22ffa5e9/base/multimedia.jl#L147-L166) so they should be equivalent. It's possible to pass a context through repr, but I didn't go that route since it seems like unnecessary extra calls for the same effect.

yurivish commented 3 years ago

OK, I'll merge this PR since it passes tests and looks like repr delegates back to show.