repl: add a bunch of types to structs and methods #22377

StefanKarpinski · 2017-06-15T06:53:20Z

No description provided.

StefanKarpinski · 2017-06-15T14:40:12Z

CI failure is a timeout.

Sacha0 · 2017-06-15T16:33:12Z

base/repl/REPL.jl


-struct LatexCompletions <: CompletionProvider; end
+mutable struct REPLCompletionProvider <: CompletionProvider end
+mutable struct ShellCompletionProvider <: CompletionProvider end


Need these two structs continue to be mutable? Above (around old lines 80-84) you changed a couple similar mutable structs to structs, so thought I might ask :).

Sacha0 · 2017-06-15T16:34:56Z

base/repl/REPL.jl

+        Base.text_colors[:red],
+        Base.text_colors[:yellow],
+        false, false, false, envcolors
+    )


Odd indentation? Perhaps better the ) simply terminate the preceding line?

This is my preferred style for writing this kind of code. Clearly not universally agreed upon, but until we have an official style guide for this, I'm going to write it the way I prefer 😬

Also happens to be my and Invenia's preferred style :)

Fair enough :).

Sacha0 · 2017-06-15T16:35:22Z

base/repl/REPL.jl

+    repl::LineEditREPL;
+    hascolor::Bool = repl.hascolor,
+    extra_repl_keymap::Vector{<:Dict} = Dict{Any,Any}[]
+)


The ) placement seems a bit unusual here as well?

This is my preferred style, although obviously not everyone agrees.

Also happens to be my and Invenia's preferred style :)

Glad to know I'm not the only one 😁

rfourquet

This is excellent, each time I tried to hack around this files I got easily lost by the lack of type annotations. I'm not export here, but as long as the test passes, this LGTM, thanks!

rfourquet · 2017-06-15T16:22:01Z

base/repl/LineEdit.jl

-    terminal
-    histprompt
+    terminal::AbstractTerminal
+    histprompt # ::HistoryPrompt


Why comment the type annotation here? (similar question in other places, e.g. with CompletionProvider)

Because that's the type in practice, but HistoryPrompt doesn't exist yet when this is defined. The reverse dependency is unfortunate and something that should be fixed, but that kind of refactoring is beyond the scope of this PR.

rfourquet · 2017-06-15T16:27:36Z

base/repl/REPL.jl


-struct LatexCompletions <: CompletionProvider; end
+mutable struct REPLCompletionProvider <: CompletionProvider end
+mutable struct ShellCompletionProvider <: CompletionProvider end


Seems you can remove the mutable here as you did for other types.

I tried that – they have finalizers, so it doesn't work.

Ah ok thanks

rfourquet · 2017-06-15T16:29:41Z

base/repl/REPL.jl

+function setup_interface(
+    repl::LineEditREPL;
+    hascolor::Bool = repl.hascolor,
+    extra_repl_keymap::Vector{<:Dict} = Dict{Any,Any}[]


did you mean ::Vector{<:Associative} instead of ::Vector{<:Dict} ?

I could do that, but in practice only Dicts get stuck in there

Then why not Vector{Dict} ? The subtype operator on a concrete type looks confusing

Oh, I see what you're saying. Yes, that makes sense, not sure what I was thinking here.

Yeah, that's funny.

Pedantic question: do we say Dict is concrete without its type parameters? (asDict != Dict{Any,Any} - and if concrete Dict{Any,Any} always finds its way here in practice, I suppose there may be a minor performance difference too).

Oh I got confused, no 100% familiar yet with this {<:T} notation ;-)

Sacha0 · 2017-06-15T16:36:53Z

base/repl/Terminals.jl

-                    `stty sane`
-                end,t.in_stream,t.out_stream,t.err_stream)
+            run((raw ? `stty raw -echo onlcr -ocrnl opost` : `stty sane`),
+                t.in_stream, t.out_stream, t.err_stream)


Sacha0 · 2017-06-15T16:37:30Z

base/repl/Terminals.jl

-            ccall(:jl_tty_set_mode,
-                 Int32, (Ptr{Void},Int32),
-                 t.in_stream.handle, raw) != -1
+            ccall(:jl_tty_set_mode, Int32, (Ptr{Void},Int32), t.in_stream.handle, raw) != -1


Missing space separating tuple arguments?

Sacha0

Superficially lgtm! :) (Modulo minor comments.)

ararslan · 2017-06-15T17:04:51Z

base/repl/Terminals.jl

@@ -113,22 +118,17 @@ cmove_right(t::UnixTerminal, n) = write(t.out_stream, "$(CSI)$(n)C")
 cmove_left(t::UnixTerminal, n) = write(t.out_stream, "$(CSI)$(n)D")
 cmove_line_up(t::UnixTerminal, n) = (cmove_up(t, n); cmove_col(t, 1))
 cmove_line_down(t::UnixTerminal, n) = (cmove_down(t, n); cmove_col(t, 1))
-cmove_col(t::UnixTerminal, n) = (write(t.out_stream, '\r'); n > 1 && cmove_right(t, n - 1))
+cmove_col(t::UnixTerminal, n) = (write(t.out_stream, '\r'); n > 1 && cmove_right(t, n-1))


Why change this?

To be consistent with the formatting in the rest of the file, which doesn't have spaces in small arithmetic expressions anywhere else.

Okay, thanks for clarifying

tkelman · 2017-06-16T08:08:04Z

base/sysimg.jl

-include("LineEdit.jl")
-include("REPLCompletions.jl")
-include("REPL.jl")
+include("repl/Terminals.jl")


using joinpath if possible may avoid mixed forward and backslashes in backtraces on windows

I finally understand why I see people use joinpath in otherwise trivial cases! Wow.

I'd kind of prefer if we used forward slashes for joinpath and everything else even on Windows but that might be unlikely. Most Windows programs can handle forward slashes in paths just fine, but maybe not all.

We should probably introduce Path types and have path"repl/Terminals.jl" produce whatever the native path syntax is. That would also allow path"C:\some\where" to work on Windows systems without double backslashes.

This matches the style of all other includes in sysimg.jl, so I'm going to leave it. Normalizing path names to included files would probably make sense. I suspect this is not simply a style issue since joinpath is not defined when much of this file runs.

StefanKarpinski · 2017-06-19T01:07:06Z

Failure is timeout and #22353, not related.

musm · 2017-06-19T01:31:17Z

base/repl/REPL.jl

@@ -1057,7 +1062,7 @@ function run_frontend(repl::StreamREPL, backend::REPLBackendRef)
    dopushdisplay && popdisplay(d)
 end

-function start_repl_server(port)
+function start_repl_server(port::Int)


::Integer ?

It's an internal function and if the value is too large, this won't work anyway, so Int is ok.

This finishes the work started in #22377

This finishes the work started in JuliaLang#22377

Sacha0 reviewed Jun 15, 2017

View reviewed changes

rfourquet approved these changes Jun 15, 2017

View reviewed changes

Sacha0 reviewed Jun 15, 2017

View reviewed changes

Sacha0 approved these changes Jun 15, 2017

View reviewed changes

ararslan reviewed Jun 15, 2017

View reviewed changes

ararslan added the REPL Julia's REPL (Read Eval Print Loop) label Jun 15, 2017

tkelman reviewed Jun 16, 2017

View reviewed changes

StefanKarpinski added 6 commits June 18, 2017 15:58

Terminals.jl: minor style tweaks

8566385

Terminals.jl: add AbstractTerminal supertype above TextTerminal

5431b5e

base/repl: collect all repl-related file under one directory

ce44871

base/repl: make some mutable structs immutable

f9a4b39

base/repl: add some types to structures

8181394

base/repl: add types on various fields and method arguments

b3e6589

StefanKarpinski force-pushed the sk/rerepl branch from eb5bf5e to b3e6589 Compare June 18, 2017 23:06

StefanKarpinski merged commit 7affd0c into master Jun 19, 2017

StefanKarpinski deleted the sk/rerepl branch June 19, 2017 01:07

musm reviewed Jun 19, 2017

View reviewed changes

timholy added a commit that referenced this pull request Jun 14, 2020

Finish adding types to fields in REPL

970d104

This finishes the work started in #22377

timholy mentioned this pull request Jun 14, 2020

Finish adding types to fields in REPL #36281

Merged

JeffBezanson pushed a commit that referenced this pull request Jun 15, 2020

Finish adding types to fields in REPL (#36281)

5853103

This finishes the work started in #22377

simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020

Finish adding types to fields in REPL (JuliaLang#36281)

6870257

This finishes the work started in JuliaLang#22377

repl: add a bunch of types to structs and methods #22377

repl: add a bunch of types to structs and methods #22377

Conversation

StefanKarpinski commented Jun 15, 2017

StefanKarpinski commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfourquet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

StefanKarpinski commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski Jun 15, 2017 •

edited

Loading

StefanKarpinski Jun 17, 2017 •

edited

Loading