-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Change keyword arguments back to regular NamedTuples #25711
Conversation
NEWS.md
Outdated
@@ -196,6 +196,11 @@ Breaking changes | |||
|
|||
This section lists changes that do not have deprecation warnings. | |||
|
|||
* Keyword arguments are now `NamedTuple`s rather than `Iterators.IndexValue`s. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't mention IndexValue
since that type isn't public.
@@ -164,7 +164,7 @@ function showerror(io::IO, ex::MethodError) | |||
ft = typeof(f) | |||
name = ft.name.mt.name | |||
f_is_function = false | |||
kwargs = () | |||
kwargs = NamedTuple() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to change the line below that was changed by #25658.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Does it look right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file in the revert is wrong. We want kwargs
to be a non-specialized container so that we don't compile a new copy of show_method_candidates
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't it already that way though? IndexValue
is also specialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not saying I necessarily managed to catch all of this regression before. In v0.6, we copied it to a Vector{Any}. That's not strictly necessary though, as long we don't pass this as an argument to something that'll cause code-generation-specialization of our error-printing code.
8e55898
to
09e9ec6
Compare
While I don't think we should ever use NT for this, if we're going to change the iteration protocol, we should at least do it with a proper deprecation (e.g. copy the IndexValue code to a KeywordsArgs type, then implement the desired behavior and deprecations on it). |
Agreed --- if we want to change this we should do a proper deprecation. |
From triage: we should rename |
That too, but I think the main request was for printing the type more concisely. Currently the default show method likes to print the full type name of the container, but in this case, most of the information there isn't useful. |
As discussed in #25711 Before: ``` julia> f(;kwargs...) = kwargs f (generic function with 1 method) julia> f(;a = 1, b = 2) Base.Iterators.IndexValue{Symbol,Int64,Tuple{Symbol,Symbol},NamedTuple{(:a, :b),Tuple{Int64,Int64}}} with 2 entries: :a => 1 :b => 2 ``` After: ``` julia> f(;a = 1, b = 2) pairs((a = 1, b = 2)) with 2 entries: :a => 1 :b => 2 ```
WIth |
Why not both (we could define |
named tuples definitely came after field access, and have no special claim to the syntax (other than currently existing, while it has not yet been defined for Pairs) |
As discussed in #25711 Before: ``` julia> f(;kwargs...) = kwargs f (generic function with 1 method) julia> f(;a = 1, b = 2) Base.Iterators.IndexValue{Symbol,Int64,Tuple{Symbol,Symbol},NamedTuple{(:a, :b),Tuple{Int64,Int64}}} with 2 entries: :a => 1 :b => 2 ``` After: ``` julia> f(;a = 1, b = 2) pairs((a = 1, b = 2)) with 2 entries: :a => 1 :b => 2 ```
As discussed in #25711 Before: ``` julia> f(;kwargs...) = kwargs f (generic function with 1 method) julia> f(;a = 1, b = 2) Base.Iterators.IndexValue{Symbol,Int64,Tuple{Symbol,Symbol},NamedTuple{(:a, :b),Tuple{Int64,Int64}}} with 2 entries: :a => 1 :b => 2 ``` After: ``` julia> f(;a = 1, b = 2) pairs((a = 1, b = 2)) with 2 entries: :a => 1 :b => 2 ```
As discussed in #25711 Before: ``` julia> f(;kwargs...) = kwargs f (generic function with 1 method) julia> f(;a = 1, b = 2) Base.Iterators.IndexValue{Symbol,Int64,Tuple{Symbol,Symbol},NamedTuple{(:a, :b),Tuple{Int64,Int64}}} with 2 entries: :a => 1 :b => 2 ``` After: ``` julia> f(;a = 1, b = 2) pairs((a = 1, b = 2)) with 2 entries: :a => 1 :b => 2 ```
Ref discussion in #25675. This reverts "undo breaking change to kwargs iteration order" (#25290) and makes keyword arguments regular ol'
NamedTuple
s again. This is breaking, sinceNamedTuple
s iterate values rather than pairs (which motivated #25290) and so iteration of keyword arguments will be broken. Users who want the(name, value)
iteration will need to usepairs
.