-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
display "(empty range)" in case of empty ranges #57697
base: master
Are you sure you want to change the base?
display "(empty range)" in case of empty ranges #57697
Conversation
@@ -618,6 +618,9 @@ function show(io::IO, r::LinRange{T}) where {T} | |||
print(io, ", ") | |||
show(io, length(r)) | |||
print(io, ')') | |||
if isempty(r) |
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 is the 2-argument show
, which should be parseable Julia code. See https://docs.julialang.org/en/v1/base/io-network/#Base.show-Tuple{IO,%20Any} .
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.
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 think it should be in a three arg show method.
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.
Yes, that's what I was getting at. From the docstring I linked:
For a more verbose human-readable text output for objects of type
T
, defineshow(io::IO, ::MIME"text/plain", ::T)
in addition.
:compact
showing is unrelated to 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.
I see now. So I assume it would be better to define show(io::IO, ::MIME"text/plain", ::T)
for range types rather than change the existing ones?
Can we tweak this so that it doesn't print when IOContext is And also try to minimize its interference with the ability to copy-paste to reconstruct an object (which might be addressed by the julia> print([1:2, 3:2, 4:5])
UnitRange{Int64}[1:2, 3:2, 4:5] I can use |
If I understand correctly, the issue is mainly with an empty |
I assume a better approach would be to define a 3-argument show ( I think it should be defined for all range types since 3-argument show is meant to display additional details and to ensure consistency. |
To take the julia> show(stdout, MIME"text/plain"(), LinRange(2.0, 4.5, 5))
5-element LinRange{Float64, Int64}:
2.0, 2.625, 3.25, 3.875, 4.5
julia> show(stdout, LinRange(2.0, 4.5, 5))
LinRange{Float64}(2.0, 4.5, 5) In the first case, the length is explicitly mentioned in the summary. In the second case, the last parameter corresponds to the length, although this isn't explicitly pointed out (as this needs to be runnable julia code). Since we're discussing the 3-argument case here, the displayed form seems to indicate the length adequately? I feel the issue is mainly with ranges where the 3-argument |
So I am trying to define a 3 argument function Base.show(io::IO, ::MIME"text/plain", r::AbstractUnitRange{T}) where T
# impl
end and I am getting this error during bootstrap:
LoadError(at "Base_compiler.jl" line 3: LoadError(at "range.jl" line 1119: LoadError(at "range.jl" line 1119: UndefVarError(var=:var"@MIME_str", world=0x0000000000000e4e, scope=Base))))
ijl_undefined_var_error at /home/prashant/Projects/julia-fork/src/rtutils.c:154
jl_eval_global_var at /home/prashant/Projects/julia-fork/src/interpreter.c:169 [inlined]
eval_value at /home/prashant/Projects/julia-fork/src/interpreter.c:223
jl_interpret_toplevel_expr_in at /home/prashant/Projects/julia-fork/src/interpreter.c:919
No code info - unknown interpreter state! I am assuming this is because maybe |
Lines 63 to 64 in 885b1cd
... so try inserting your method somewhere after that point, maybe try one of these files: Lines 112 to 113 in 885b1cd
|
As mentioned in #40331, empty ranges could be confusing. This pr adds a message "(empty range)" incase if the range is empty.
So for the example given in the issue