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

mention methodtable Ctrl+Q trick in methodshow #35556

Merged
merged 7 commits into from
Jun 14, 2020

Conversation

simeonschaub
Copy link
Member

@simeonschaub simeonschaub commented Apr 22, 2020

Thanks to @oxinabox for pointing me to this trick on Slack. I think this deserves to be mentioned in the MethodTable output.

@simeonschaub
Copy link
Member Author

Tagging @fredrikekre, since I believed you added this feature in #22007. Could I get a review?

@tkf
Copy link
Member

tkf commented May 10, 2020

This is a very nice thing to have for new users. But wouldn't this write the "tips" message into a file if I type open(io -> show(io, methods(identity)), "file", write=true)? I think "UI" message like this should be implemented in the REPL module (e.g., REPL sets some custom IOContext and it'd be used here instead of isinteractive()).

@simeonschaub
Copy link
Member Author

Should I check for haskey(io, :module) instead? Otherwise, I don't think the REPL module sets any custom IOContexts.

@tkf
Copy link
Member

tkf commented May 10, 2020

I don't think :module context is adequate for this. Why not add a new IOContext like this?

diff --git a/stdlib/REPL/src/REPL.jl b/stdlib/REPL/src/REPL.jl
index 30af00a6cd..be3e14538d 100644
--- a/stdlib/REPL/src/REPL.jl
+++ b/stdlib/REPL/src/REPL.jl
@@ -206,6 +206,7 @@ end
 function display(d::REPLDisplay, mime::MIME"text/plain", x)
     io = outstream(d.repl)
     get(io, :color, false) && write(io, answer_color(d.repl))
+    io = IOContext(io, :interactive => true)
     if isdefined(d.repl, :options) && isdefined(d.repl.options, :iocontext)
         # this can override the :limit property set initially
         io = foldl(IOContext, d.repl.options.iocontext,

@tkf tkf added display and printing Aesthetics and correctness of printed representations of objects. REPL Julia's REPL (Read Eval Print Loop) labels May 11, 2020
@tkf tkf requested a review from KristofferC May 11, 2020 22:31
@tkf
Copy link
Member

tkf commented May 11, 2020

If we go with this direction, should we add :interactive to this list?

julia/base/show.jl

Lines 266 to 283 in 65869b1

- `:compact`: Boolean specifying that small values should be printed more compactly, e.g.
that numbers should be printed with fewer digits. This is set when printing array
elements.
- `:limit`: Boolean specifying that containers should be truncated, e.g. showing `…` in
place of most elements.
- `:displaysize`: A `Tuple{Int,Int}` giving the size in rows and columns to use for text
output. This can be used to override the display size for called functions, but to
get the size of the screen use the `displaysize` function.
- `:typeinfo`: a `Type` characterizing the information already printed
concerning the type of the object about to be displayed. This is mainly useful when
displaying a collection of objects of the same type, so that redundant type information
can be avoided (e.g. `[Float16(0)]` can be shown as "Float16[0.0]" instead
of "Float16[Float16(0.0)]" : while displaying the elements of the array, the `:typeinfo`
property will be set to `Float16`).
- `:color`: Boolean specifying whether ANSI color/escape codes are supported/expected.
By default, this is determined by whether `io` is a compatible terminal and by any
`--color` command-line flag when `julia` was launched.

@simeonschaub
Copy link
Member Author

Yes, I think that would be helpful. Should this all go into this PR or would it make more sense to have a dedicated PR for this?

@tkf
Copy link
Member

tkf commented May 11, 2020

This patch introduces a new convention in REPL.jl so I think it makes sense to add the documentation here.

@simeonschaub
Copy link
Member Author

Perhaps REPL or isREPL is a better name for this, since this trick doesn't work in Jupyter Notebooks, for example, but there isinteractive() still returns true?

@tkf
Copy link
Member

tkf commented May 12, 2020

Ah, that's a very good point. Yes, :interactive context is useless for this.

I wonder if we should just directly overload display(d::REPLDisplay, mime::MIME"text/plain", x) or add some overloadable hint mechanism.

A bit hacky solution may be something like

function display(d::REPLDisplay, mime::MIME"text/plain", x)
    backup = copy(Base.LAST_SHOWN_LINE_INFOS)
    empty!(Base.LAST_SHOWN_LINE_INFOS)

    ...

    if isempty(Base.LAST_SHOWN_LINE_INFOS)
        append!(Base.LAST_SHOWN_LINE_INFOS, backup)
    else
        print(io, the_hint_message)
    end
end

@tkf
Copy link
Member

tkf commented May 12, 2020

Hmm... Actually, maybe LAST_SHOWN_LINE_INFOS itself should be in IOContext? It's not nice to communicate via a mutating global state. I think it'd be better to do something like

infos = []
io = IOContext(io, :LAST_SHOWN_LINE_INFOS => infos)
show(io, "text/plain", x)
d.repl.last_shown_line_infos = infos

in display.

@simeonschaub
Copy link
Member Author

I don't really get, why we would need Base.LAST_SHOWN_LINE_INFOS here. Does this tell us anything about whether the output is a REPL? Is there anything wrong with just using the approach here and renaming :interactive to :REPL?

@tkf
Copy link
Member

tkf commented May 15, 2020

Does this tell us anything about whether the output is a REPL?

No, it doesn't. My suggestion was to do it the other way around. If we implement this in display(d::REPLDisplay, mime::MIME"text/plain", x), we know that the output is the REPL because of d::REPLDisplay. However, now we need to know if the method table is printed or not. If we do empty!(Base.LAST_SHOWN_LINE_INFOS) before calling show and then if it becomes non-empty after that, we know that a method table is printed.

(However, this is correct only if there is only one Task. It's much better to do this without the global variable Base.LAST_SHOWN_LINE_INFOS now that threading (or even @async) becomes more common. So that's why I'm mentioning that using IOContext is even better.)

I think this is the right way to do it. As you pointed out, this is very specific to REPL. So, I think REPL is the best place to implement it.

@simeonschaub
Copy link
Member Author

@tkf I got the same error locally, I just wanted to see whether it was just on my machine. Do you have any idea why it would fail? I don't really know where to start looking into this.

stdlib/REPL/src/REPL.jl Outdated Show resolved Hide resolved
Copy link
Member

@tkf tkf left a comment

Choose a reason for hiding this comment

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

Isn't it simpler to always use io = IOContext(io, :LAST_SHOWN_LINE_INFOS => infos)?

Also, current code seems to not update d.repl.last_shown_line_infos after the second time?

stdlib/REPL/src/REPL.jl Outdated Show resolved Hide resolved
stdlib/REPL/src/REPL.jl Show resolved Hide resolved
@simeonschaub
Copy link
Member Author

Yes, that makes sense. Originally, I had structured it a bit differently, but doesn't really matter either way.

Co-authored-by: Takafumi Arakaki <takafumi.a@gmail.com>
stdlib/REPL/src/REPL.jl Outdated Show resolved Hide resolved
Co-authored-by: Takafumi Arakaki <takafumi.a@gmail.com>
@simeonschaub
Copy link
Member Author

Bump. Is this good to go?

@tkf tkf requested a review from StefanKarpinski June 3, 2020 20:38
@tkf
Copy link
Member

tkf commented Jun 3, 2020

It's is rather a radical change in the "philosophy" of how display works on REPL. So it'd be nice if other devs can have a look at it.

ping @StefanKarpinski

@StefanKarpinski
Copy link
Member

I don't think I'm the person for this particular review. @JeffBezanson, @stevengj and @vtjnash have more of a history and familiarity with the display system.

@tkf tkf requested review from JeffBezanson, stevengj and vtjnash and removed request for StefanKarpinski and KristofferC June 4, 2020 16:42
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Ah, this is rather clever! It's unusual, but I can't see any reason that'd be a problem.

Comment on lines +634 to +636
if haskey(io, :LAST_SHOWN_LINE_INFOS)
resize!(io[:LAST_SHOWN_LINE_INFOS], 0)
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if haskey(io, :LAST_SHOWN_LINE_INFOS)
resize!(io[:LAST_SHOWN_LINE_INFOS], 0)
end

Copy link
Member

Choose a reason for hiding this comment

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

Is it OK to skip resize! here? What happens when multiple stack traces are shown (via nested errors)? Would IOContext be reset appropriately?

@@ -373,6 +374,7 @@ show(io::IO, mime::MIME"text/html", mt::Core.MethodTable) = show(io, mime, Metho

# pretty-printing of AbstractVector{Method}
function show(io::IO, mime::MIME"text/plain", mt::AbstractVector{Method})
LAST_SHOWN_LINE_INFOS = get(io, :LAST_SHOWN_LINE_INFOS, Tuple{String,Int}[])
resize!(LAST_SHOWN_LINE_INFOS, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
resize!(LAST_SHOWN_LINE_INFOS, 0)

Copy link
Member

Choose a reason for hiding this comment

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

Don't you need resize! for something like this?

struct TwoMethodTables
    methods1
    methods2
end

function Base.show(io::IO, ::MIME"text/plain", two::TwoMethodTables)
    println(io, "TwoMethodTables:")
    show(io, MIME"text/plain"(), two.methods1)
    show(io, MIME"text/plain"(), two.methods2)
end

Currently, I think Ctrl+Q works for numbering in methods2. If we remove resize!, it'd refer to the numbering in methods1 (and also in methods2 shifted by length(methods)).

@@ -240,6 +240,7 @@ function show_method_table(io::IO, ms::MethodList, max::Int=-1, header::Bool=tru
end
n = rest = 0
local last
LAST_SHOWN_LINE_INFOS = get(io, :LAST_SHOWN_LINE_INFOS, Tuple{String,Int}[])

resize!(LAST_SHOWN_LINE_INFOS, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
resize!(LAST_SHOWN_LINE_INFOS, 0)

stdlib/REPL/src/REPL.jl Outdated Show resolved Hide resolved
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@simeonschaub
Copy link
Member Author

Bumping this again. I thought the resize! was still necessary too, so printing multiple stack traces still works correctly.

@tkf tkf merged commit 13b07fc into JuliaLang:master Jun 14, 2020
@tkf
Copy link
Member

tkf commented Jun 14, 2020

I merged this as @vtjnash is OK with it at a high-level picture. I'm keeping resize! since both @simeonschaub and I think that's necessary.

@vtjnash Let us know if the code around resize! still has to be fixed.

It seems to be better to merge now so that there is more chance to play with the UI in the 1.6-DEV cycle. It's also better to merge this before #36134.

(The test failures look like some network glitches.)

@tkf tkf mentioned this pull request Jul 1, 2020
simeonschaub added a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
Co-authored-by: Takafumi Arakaki <takafumi.a@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects. REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants