Keyword display improvements #16669

dhoegh · 2016-05-30T19:34:43Z

This PR implements several improvement to how keywords is presented to the user. The additions in each commit is listed bellow

1. adds display of variable keyword arguments such as kwargs.... This change causes the vararg keywords to be displayed in method list and other places.
1. adds the display of keywords to method completion

julia> split(<tab>
split{T<:SubString{T<:AbstractString}}(str::T, splitter; limit, keep) at strings/util.jl:126
split(str::AbstractString) at strings/util.jl:151
split{T<:AbstractString}(str::T, splitter; limit, keep) at strings/util.jl:127

1. Make the generic error thrown by kwerr throw a MethodError. This allows for display of other method candidates, handled by the fourth commit.
1. Add so method errors caused by keywords is displayed as the original method, with a text stating if the keyword didn't match. This fixes MethodError message misleading/wrong on 0.4, very hard to read on 0.5 #15639

Example from #15639:

type FooCon
    x
end
type BarCon
    x
end
function addConstraint(c::FooCon)
    println("addCon(FooCon)")
end
function addConstraint(c::BarCon; uncset=nothing)
    println("addCon(BarCon,$uncset)")
    # Oh, BarCon was suppossed to be a FooCon, let me erroneously pass
    # on this kwarg which isn't supported
    addConstraint(FooCon(c.x), uncset=uncset)
end
julia> addConstraint(BarCon(1))
addCon(BarCon,nothing)
ERROR: MethodError: no method matching addConstraint(::FooCon; uncset=nothing)
Closest candidates are:
  addConstraint(::FooCon) got an unrecognized keyword argument "uncset"
  addConstraint(::BarCon; uncset)
 in #addConstraint#1(::Void, ::Function, ::BarCon) at ./REPL[4]:5
 in addConstraint(::BarCon) at ./REPL[4]:2
 in eval(::Module, ::Any) at ./boot.jl:225
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

Note to reviewer please check if the changes in src/julia-syntax.scm is correct as this is my first encounter with lisp.

IainNZ · 2016-05-30T19:53:25Z

@dhoegh can you post the new error message for that case?

quinnj · 2016-05-30T19:54:12Z

A big +1 to progress with this. In the method lists, I wonder if we should display the default kwarg value too? Seems like that's usually the next question once I know there is a keyword argument involved.

Also probably want to track the updates in #16580 with how keyword arguments will be handled internally.

dhoegh · 2016-05-30T19:55:55Z

I have updated the example.

vtjnash · 2016-05-30T20:51:03Z

base/replutil.jl

@@ -123,6 +123,16 @@ function showerror(io::IO, ex::MethodError)
    ft = typeof(f)
    name = ft.name.mt.name
    f_is_function = false
+    kwargs = Dict{Any,Any}()


i think you meant Array{Any}(0) here?

Yes, thank you. I have corrected it.

dhoegh · 2016-05-31T18:27:31Z

I have fixed the comment from vtnash. I have taken a brief look at #16580 and the approach used here seems to be adaptable for the PR. Is #16580 slated 0.5 material, because I do think it would be good to get this in 0.5.

nalimilan · 2016-05-31T20:38:29Z

Great!

Though for consistency with positional arguments, the value of keyword arguments shouldn't be printed on MethodError. So this:

ERROR: MethodError: no method matching addConstraint(::FooCon; uncset=nothing)

would rather be:

ERROR: MethodError: no method matching addConstraint(::FooCon; uncset::Void)

Also, maybe "unsupported" would be better than "unrecognized".

IainNZ · 2016-05-31T21:26:07Z

@nalimilan why would it be Void? Surely its Any?

nalimilan · 2016-05-31T21:36:50Z

@IainNZ AFAIK the displayed type is that of the passed argument, not the one in the signature (indeed, that argument doesn't even exist for that method, so it doesn't have a type).

IainNZ · 2016-05-31T21:54:53Z

That... doesn't make any sense then, right? Doesn't seem like something to perpetuate

…eywords.

…ismatch

tkelman · 2016-06-04T12:08:21Z

base/replutil.jl

@@ -150,6 +160,13 @@ function showerror(io::IO, ex::MethodError)
            print(io, "::$typ")
            i == length(arg_types_param) || print(io, ", ")
        end
+        if 0 < length(kwargs)


use isempty instead of comparing length to 0

dhoegh · 2016-06-05T12:19:14Z

I have addressed the comment from tkelman and changed "unrecognized" to "unsupported" based on nalimilan comment.

Regarding display of default values, that seem to require more than a trivial amount of look through the AST of the kwsorter function. Therefore I do not think it is appropriate to implement it here, but it would be a nice addition.

dhoegh · 2016-06-13T16:50:34Z

Bump, any further comments or thoughts.

vtjnash · 2016-06-23T02:54:34Z

base/replutil.jl

+        if !isempty(kwargs)
+            print(io, "; ")
+            for (i ,(k, v)) in enumerate(kwargs)
+                print(io, k, "=", v)


use show(IOContext(io, :limit => true), v), not print(io, v) for v

(also the \space, are in the wrong order on the previous line)

vtjnash · 2016-06-23T03:19:04Z

sorry this took so long to review. I've been coming back to this every few days trying to make sure I was understanding how everything would interact.

Can I ask you to add a test for the behavior with multiple kwargs (multiple matching, multiple not matching)?

StefanKarpinski · 2016-06-23T03:57:56Z

Given that this is a display change, which tends to be less likely to break people's code, can't we merge this and then patch it up?

…method in show_method_candidates with a text stating the not matching keyword. Fix JuliaLang#15639

dhoegh · 2016-06-26T09:10:00Z

I have fixed the trivial comments and added test for multiple kwargs matching and multiple not matching at line https://github.com/dhoegh/julia/blob/d4e8792075735c9a71e52f3ddad62cb96bf589ad/test/replutil.jl#L112-L115. I have squashed the commit where the review comments was addressed away. My time is very limited right now, so my response time the next couple of weeks could be long.

vtjnash · 2016-06-28T16:50:58Z

Thanks. This is great.

dhoegh mentioned this pull request May 30, 2016

MethodError message misleading/wrong on 0.4, very hard to read on 0.5 #15639

Closed

dhoegh force-pushed the kword branch from 276670d to a1118cb Compare May 30, 2016 19:38

dhoegh force-pushed the kword branch from a1118cb to 73b96cb Compare May 30, 2016 19:59

vtjnash reviewed May 30, 2016
View reviewed changes

dhoegh force-pushed the kword branch from 73b96cb to 89e278e Compare May 31, 2016 17:41

dhoegh force-pushed the kword branch 2 times, most recently from 1091f8a to e808e07 Compare May 31, 2016 19:22

dhoegh added 3 commits June 4, 2016 13:57

Add printing of kwargs... if the function accept variable number of k…

e48d9a3

…eywords.

Method completion with keywords.

c499d4c

Throw method error instead of throwing a generic error upon keyword m…

2e1ed5d

…ismatch

dhoegh force-pushed the kword branch from e808e07 to 02a157a Compare June 4, 2016 11:57

tkelman reviewed Jun 4, 2016
View reviewed changes

dhoegh force-pushed the kword branch 3 times, most recently from 069b03c to 673826b Compare June 5, 2016 12:09

vtjnash reviewed Jun 23, 2016
View reviewed changes

dhoegh force-pushed the kword branch 2 times, most recently from c353489 to f49fcb9 Compare June 26, 2016 08:56

Fix so method errors caused by keywords is displayed as the original …

d4e8792

…method in show_method_candidates with a text stating the not matching keyword. Fix JuliaLang#15639

dhoegh force-pushed the kword branch from f49fcb9 to d4e8792 Compare June 26, 2016 08:57

vtjnash merged commit c775fc5 into JuliaLang:master Jun 28, 2016

JeffBezanson mentioned this pull request Aug 6, 2017

Bad error message when calling keyword-less constructors with keywords #17207

Closed

skleinbo mentioned this pull request May 10, 2022

Avoid printing values of keyword arguments while displaying a MethodError #45219

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keyword display improvements #16669

Keyword display improvements #16669

dhoegh commented May 30, 2016 •

edited

Loading

IainNZ commented May 30, 2016

quinnj commented May 30, 2016

dhoegh commented May 30, 2016

vtjnash May 30, 2016

dhoegh May 31, 2016

dhoegh commented May 31, 2016

nalimilan commented May 31, 2016

IainNZ commented May 31, 2016

nalimilan commented May 31, 2016

IainNZ commented May 31, 2016 •

edited

Loading

tkelman Jun 4, 2016

dhoegh commented Jun 5, 2016

dhoegh commented Jun 13, 2016

vtjnash Jun 23, 2016

vtjnash commented Jun 23, 2016

StefanKarpinski commented Jun 23, 2016

dhoegh commented Jun 26, 2016

vtjnash commented Jun 28, 2016

Keyword display improvements #16669

Keyword display improvements #16669

Conversation

dhoegh commented May 30, 2016 • edited Loading

IainNZ commented May 30, 2016

quinnj commented May 30, 2016

dhoegh commented May 30, 2016

vtjnash May 30, 2016

Choose a reason for hiding this comment

dhoegh May 31, 2016

Choose a reason for hiding this comment

dhoegh commented May 31, 2016

nalimilan commented May 31, 2016

IainNZ commented May 31, 2016

nalimilan commented May 31, 2016

IainNZ commented May 31, 2016 • edited Loading

tkelman Jun 4, 2016

Choose a reason for hiding this comment

dhoegh commented Jun 5, 2016

dhoegh commented Jun 13, 2016

vtjnash Jun 23, 2016

Choose a reason for hiding this comment

vtjnash commented Jun 23, 2016

StefanKarpinski commented Jun 23, 2016

dhoegh commented Jun 26, 2016

vtjnash commented Jun 28, 2016

dhoegh commented May 30, 2016 •

edited

Loading

IainNZ commented May 31, 2016 •

edited

Loading