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

use Base.show #165

Merged
merged 2 commits into from
Jan 28, 2019
Merged

use Base.show #165

merged 2 commits into from
Jan 28, 2019

Conversation

kleinschmidt
Copy link
Contributor

@kleinschmidt kleinschmidt commented Jan 18, 2019

There's one place where a show method is defined that's not Base.show. I get errors like this

julia> weave("cogsci19/cogsci.jmd", mod=Main)
[ Info: Weaving chunk 1 from line 1
ERROR: MethodError: no method matching show(::Base.GenericIOBuffer{Array{UInt8,1}}, ::MIME{Symbol("text/plain")}, ::Symbol)
You may have intended to import Base.show
Closest candidates are:
  show(::IO, ::MIME{Symbol("text/html")}, ::Markdown.MD) at /home/dave/.julia/dev/Weave/src/Markdown2HTML.jl:230
Stacktrace:
 [1] #sprint#324(::Nothing, ::Int64, ::Function, ::Function, ::MIME{Symbol("text/plain")}, ::Vararg{Any,N} where N) at ./strings/io.jl:101
 [2] sprint(::Function, ::MIME{Symbol("text/plain")}, ::Vararg{Any,N} where N) at ./strings/io.jl:97
 [3] tohtml(::Base.GenericIOBuffer{Array{UInt8,1}}, ::MIME{Symbol("text/plain")}, ::Symbol) at /home/dave/.julia/dev/Weave/src/Markdown2HTML.jl:12
 [4] tohtml(::Base.GenericIOBuffer{Array{UInt8,1}}, ::Symbol) at /home/dave/.julia/dev/Weave/src/Markdown2HTML.jl:34
 [5] html(::Base.GenericIOBuffer{Array{UInt8,1}}, ::Symbol) at /home/dave/.julia/dev/Weave/src/Markdown2HTML.jl:164
 [6] html(::Base.GenericIOBuffer{Array{UInt8,1}}, ::Array{Any,1}) at /home/dave/.julia/dev/Weave/src/Markdown2HTML.jl:87
 [7] html(::Base.GenericIOBuffer{Array{UInt8,1}}, ::Markdown.MD) at /home/dave/.julia/dev/Weave/src/Markdown2HTML.jl:92
 [8] #sprint#324(::Nothing, ::Int64, ::Function, ::Function, ::Markdown.MD) at ./strings/io.jl:101
 [9] sprint at ./strings/io.jl:97 [inlined]
 [10] html(::Markdown.MD) at /home/dave/.julia/dev/Weave/src/Markdown2HTML.jl:227
 [11] format_chunk(::Weave.DocChunk, ::Dict{Symbol,Any}, ::Weave.JMarkdown2HTML) at /home/dave/.julia/dev/Weave/src/format.jl:132
 [12] format(::Weave.WeaveDoc) at /home/dave/.julia/dev/Weave/src/format.jl:33
 [13] #weave#10(::Symbol, ::Symbol, ::Symbol, ::Dict{Any,Any}, ::Module, ::String, ::Nothing, ::String, ::Symbol, ::Bool, ::Nothing, ::Nothing, ::Nothing, ::Array{String,1}, ::String, ::typeof(weave), ::String) at /home/dave/.julia/dev/Weave/src/Weave.jl:112
 [14] (::getfield(Weave, Symbol("#kw##weave")))(::NamedTuple{(:mod,),Tuple{Module}}, ::typeof(weave), ::String) at ./none:0
 [15] top-level scope at none:0

My hunch is that somehow the one non-Base show method being defined is somehow blocking the Base ones, but I could be wrong. Changing the function definition to be Base.show fixes the error but then I get warnings like

julia> using Weave
[ Info: Recompiling stale cache file /home/dave/.julia/compiled/v1.0/Weave/9EzOc.ji for Weave [44d3d7a6-8a23-5bf8-98c5-b353f8df5ec9]
WARNING: Method definition show(IO, Base.MIME{Symbol("text/html")}, Markdown.MD) in module Markdown at /build/julia/src/julia-1.0.3/usr/share/julia/stdlib/v1.0/Markdown/src/render/html.jl:188 overwritten in module Markdown2HTML at /home/dave/.julia/dev/Weave/src/Markdown2HTML.jl:230.
WARNING: Method definition latex(IO, Markdown.LaTeX) in module Markdown at /build/julia/src/julia-1.0.3/usr/share/julia/stdlib/v1.0/Markdown/src/IPython/IPython.jl:28 overwritten in module Weave at /home/dave/.julia/dev/Weave/src/format.jl:329.

So maybe this isn't the right approach but I'm proposing it here just in case. @dmbates pointed this out in #149 (even though it didn't address the underlying problem there).

@coveralls
Copy link

coveralls commented Jan 18, 2019

Coverage Status

Coverage decreased (-0.4%) to 65.004% when pulling e4353e7 on kleinschmidt:dfk/baseshow into 921b9ec on mpastell:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 65.523% when pulling b748167 on kleinschmidt:dfk/baseshow into 921b9ec on mpastell:master.

@mpastell
Copy link
Collaborator

What happens if you remove the entire method? I think the definition might be identical to the one in base...

@kleinschmidt
Copy link
Contributor Author

kleinschmidt commented Jan 19, 2019 via email

@kleinschmidt
Copy link
Contributor Author

Tests seem to pass without that method. There's another place where it seems a method is being clobbered:

WARNING: Method definition latex(IO, Markdown.LaTeX) in module Markdown at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Markdown/src/IPython/IPython.jl:28 overwritten in module Weave at /home/dave/.julia/packages/Weave/1O5IK/src/format.jl:329.

There the methods don't seem to be identical: the Weave.jl one uses a \begin{align}...\end{align} whereas the stdlib one uses $$...$$ to delimit inserted latex. Tests still pass if the Weave.jl method is removed but I'm not sure whether a) that behavior is actually tested or b) the difference with the stdlib method is intentional.

@mpastell
Copy link
Collaborator

Thanks for the fix and checking! I'll merge now.

The overwrite of latex(IO, Markdown.LaTeX) is intentional and it was added to fix #142 . It would be better not to replace base methods and I'll try to fix it at some point.

@mpastell mpastell merged commit d0c71ba into JunoLab:master Jan 28, 2019
@greimel greimel mentioned this pull request Jan 28, 2019
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.

3 participants