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

rewrite show as writemime for Julia v0.4 #219

Merged
merged 7 commits into from
Jun 15, 2016

Conversation

dancasimiro
Copy link
Contributor

writemime was deprecated in JuliaLang/julia#16563 in julia commit
ae62bf0b813afbf32402874451e55d16de909bd4. This compatibility change
allows code that is written in the Julia v0.5 style to continue working
on Julia v0.4. I have not tested this on Julia v0.3.

The compat macro must be used on the import statement and the function
definition. For example:

@compat import Base.show
@compat show(io::IO, ::MIME"text/plain", ::TestCustomShowType) = print(io, "MyTestCustomShowType")

writemime was deprecated in JuliaLang/julia#16563 in julia commit
ae62bf0b813afbf32402874451e55d16de909bd4. This compatibility change
allows code that is written in the Julia v0.5 style to continue working
on Julia v0.4. I have not tested this on Julia v0.3.

The @compat macro must be used on the import statement and the function
definition. For example:

@compat import Base.show
@compat show(io::IO, ::MIME"text/plain", ::TestCustomShowType) = print(io, "MyTestCustomShowType")
@TotalVerb
Copy link
Contributor

TotalVerb commented Jun 2, 2016

Could you also support the syntax @compat Base.show([...]) = [...], which would be rewritten to Base.writemime([...]) = [...]?

@TotalVerb
Copy link
Contributor

TotalVerb commented Jun 2, 2016

Also, might it be easier simply to define a generic writemime method that delegates to show? I don't know if this will cause problems; maybe ambiguity?

Base.writemime{M}(io::IO, ::Type{MIME{M}}, x) = Base.show(io, MIME{M}, x)

@stevengj
Copy link
Member

stevengj commented Jun 2, 2016

In 0.5, you are supposed to define show(io::IO, ::TestCustomShowType), not show(io::IO, ::MIME"text/plain", ::TestCustomShowType), for the text/plain type , so @compat should support the two-argument form too.

@stevengj
Copy link
Member

stevengj commented Jun 2, 2016

@TotalVerb, Base no longer calls writemime at all, so adding another writemime method won't help.

@TotalVerb
Copy link
Contributor

TotalVerb commented Jun 2, 2016

@stevengj I meant that it may be possible for that code to be included in Compat for older versions. Then one can just define a method of show and the fallback writemime will call it.

There don't seem to be any three-argument shows in Julia v0.3 (haven't checked 0.4), so packages that extend Base.show with three-argument methods should be able to do so with no problems. Then when code calls writemime, and no writemime is defined, it can fall back to show, for which three-argument methods have been defined.

@@ -438,6 +442,8 @@ function _compat(ex::Expr)
rewrite_pairs_to_tuples!(ex)
elseif VERSION < v"0.4.0-dev+1246" && f == :String
ex = Expr(:call, :bytestring, ex.args[2:end]...)
elseif length(ex.args) == 4 && ex.args[1] === :show
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be version dependent otherwise it's calling a deprecated function on 0.5

@TotalVerb
Copy link
Contributor

TotalVerb commented Jun 2, 2016

The below code snippet seems to work in some situations, but sometimes more code is needed for it to work cleanly. It might be better to just to go along with the proposal here, though @compat in front of Base.show extensions look ugly to me.

# Provide writemime ⇒ show fallback
if VERSION < v"0.5.0-dev+4356"
    Base.show(x::IO, y::AbstractString, z) = show(x, MIME(y), z)
    Base.writemime(x::IO, y, z) = show(x, y, z)
end

@dancasimiro
Copy link
Contributor Author

@TotalVerb: I tried defining the writemime to show fallback that you suggested, but I couldn't get it working. The compiler never dispatched to my custom implementation. As a result, the string comparison in the new tests failed.

I don't think that Julia v0.4 supports a two argument variant of writemime. As a result,
support for the two argument version of show inserts an explicit reference to the
text/plain MIME type into the argument list.
@TotalVerb
Copy link
Contributor

TotalVerb commented Jun 2, 2016

@dancasimiro Do you have code that demonstrates the problem? I have a similar solution in place on Currencies.jl's master branch, and it works for my (very limited) use case.

@stevengj
Copy link
Member

stevengj commented Jun 3, 2016

@TotalVerb, that will (a) break mimewritable (which is used e.g. by IJulia to detect which mime formats are available for a given type); (b) not work if there is an more specific writemime method (such as the one for AbstractVector) if you want to override it for an even more specific type (e.g. your own vector type); and (c) does not seem like it should work even for overriding the default writemime(io::IO, ::MIME"text/plain", x) = show(io, x) that is already in Base in 0.4, though if you define a 2-argument show for text/plain it will continue to work (unless there is already a more specific writemime, as for AbstractVector).

I really think we need a @compat show(io, ...) = .... rewriting macro to cover all of the bases here.

@TotalVerb
Copy link
Contributor

Point taken. I think the solution in this PR is best then.

I forgot to add the version check to the import handling check in addition
to the function handling.
@dancasimiro
Copy link
Contributor Author

The new tests that I wrote are passing on 0.3, 0.4, and 0.5. Is there anything else that I need to address before squashing and rebasing?

@TotalVerb
Copy link
Contributor

I still would like a Base.show rewriting, but having something is good. 👍

@nalimilan
Copy link
Member

Could you add a short explanation to README.md?

Show the new @compat support for rewriting "show" as "writemime."
@timholy timholy merged commit 9bc163c into JuliaLang:master Jun 15, 2016
@timholy
Copy link
Member

timholy commented Jun 15, 2016

Thanks @dancasimiro!

@stevengj
Copy link
Member

stevengj commented Aug 4, 2016

Note that we should also have a show(io::IO, m, x) = writemime(io, m, x) method on 0.4 for people that want to call the three-argument show, not to define new methods. You shouldn't be forced to call @compat for this.

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 this pull request may close these issues.

6 participants