Rename IndexValue -> Pairs, print it better #25764
Conversation
ararslan
added
the
domain:collections
base/essentials.jl
Outdated
|
|
||
| # forward declarations for use by iterators.jl (since parts of that file | ||
| # are used by the compiler) | ||
| function showarg end |
There was a problem hiding this comment.
editor forgetting to add newlines?
base/iterators.jl
Outdated
| print(io, "pairs(", r.data, ")") | ||
| else | ||
| print(io, "Iterators.Pairs(",r.data, ", ", r.itr,")") | ||
| end | ||
| end |
There was a problem hiding this comment.
I think we usually put these definitions in show.jl to avoid the ordering issue. Also, missing a few spaces after the commas
base/iterators.jl
Outdated
| end | ||
|
|
||
| function Base.showarg(io::IO, r::Pairs, toplevel) | ||
| if try typeof(pairs(r.data)) == typeof(r); catch; false; end |
There was a problem hiding this comment.
Why is there a try/catch trying to hide on this line?
There was a problem hiding this comment.
This seems like a bad way to handle this.
There was a problem hiding this comment.
Seemed like pairs might throw?
There was a problem hiding this comment.
Well, this is an interactive-use function, so I didn't think it was too-terrible. The query is basically "Is this the type I'd get if I called pairs on this thing". Seemed like the most straightforward way to express that. We could also do something with return_types, but not sure that's better.
There was a problem hiding this comment.
I had a great reply typed out, then clicked the wrong button. C'est la vie, I suppose – second draft is always better, right?
I looked at how the other showarg functions are implemented, and noticed / realized that none of them care about whether the printed syntax is precise, just whether it is useful. That is to say, they only care if its a reasonable interpretation. None of them care to check if that interpretation is precise.
In conclusion, the implementation should simply be:
print(io, "pairs(")
showarg(io, r.data, false)
print(io, ")")There was a problem hiding this comment.
And also define:
showarg(io, nt::NamedTuple, toplevel) = print(io, toplevel ? typeof(nt) : "::NamedTuple")
showarg(io, tt::Tuple, toplevel) = print(io, toplevel ? typeof(tt) : "::Tuple")There was a problem hiding this comment.
In conclusion, the implementation should simply be:
Well, The Pairs iterator allows different index sets, so e.g. pairs(IndexCartesian(), A) should not print as pairs(A).
There was a problem hiding this comment.
I think the same could be said of the other showarg functions. Mostly, they'll be distinguished by the printed data. But we can just extend this to handle that case also.
There was a problem hiding this comment.
I don't think so. If we print a simple pairs( line, we should make sure it's correct. We wouldn't print ReshapedArray as reshape(::Array, rand(Int), rand(Int)).
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:
After: