Add trailing nl for display(::TextDisplay, ...)

[t-bltg]

Fix #43766.

Test pass locally on 1.6.5 and 1.7.1 builds on linux, let's check CI now ...

It'd be great (if possible) to have this land in 1.6 and 1.7.

[mkitti]

Test Failed at /usr/home/julia/buildbot/w2_tester/tester_freebsd64/build/share/julia/stdlib/v1.8/DelimitedFiles/test/runtests.jl:327
  Expression: String(take!(d.io)) == "3,1,4\n"
   Evaluated: "3,1,4\n\n" == "3,1,4\n"

[t-bltg]

Thanks, I forgot to test the stdlib modules (only Base), will fix that asap.

[t-bltg]

Looks like the CI failure is caused by #43059.

Must wait for #43790 to be merged.

[xitology]

The change itself may be reasonable, but it's going to break all the code that uses display for output.

[mkitti]

The change itself may be reasonable, but it's going to break all the code that uses display for output.

I would be interested in learning more about code that uses display for output using TextDisplay. Do you have examples?

[xitology]

I use display all the time in command-line scripts to print multi-line data. I have to add an extra println() after each display() call, which I admit is irritating.

Here is my package where this change broke the test suite:
https://github.com/MechanicalRabbit/NarrativeTest.jl/runs/4831849293?check_suite_focus=true
The specific line in the package that calls display:
https://github.com/MechanicalRabbit/NarrativeTest.jl/blob/master/src/NarrativeTest.jl#L380

[t-bltg]

I use display all the time in command-line scripts to print multi-line data. I have to add an extra println() after each display() call, which I admit is irritating.

I guess you should use show instead of display.

[xitology]

I use display all the time in command-line scripts to print multi-line data. I have to add an extra println() after each display() call, which I admit is irritating.

I guess you should use show instead of display.

show(stdout, "text/plain", x) is a bit mouthful compared with display(x).

[t-bltg]

IMO having a consistent behavior for display using either TextDisplay or REPLDisplay (as was the motivation of this PR (see #43766)) is the right move.

Isn't show(stdout, x) sufficient in your case ?

[xitology]

IMO having a consistent behavior for display using either TextDisplay or REPLDisplay (as was the motivation of this PR) is the right move.

Isn't show(stdout, x) sufficient in your case ?

It gives a different output:

julia> show(stdout, [1,2,3])
[1, 2, 3]
julia> show(stdout, "text/plain", [1,2,3])
3-element Vector{Int64}:
 1
 2
 3
julia> display([1,2,3])
3-element Vector{Int64}:
 1
 2
 3

julia> show(stdout, Dict("key" => "value"))
Dict("key" => "value")
julia> show(stdout, "text/plain", Dict("key" => "value"))
Dict{String, String} with 1 entry:
  "key" => "value"
julia> display(Dict("key" => "value"))
Dict{String, String} with 1 entry:
  "key" => "value"

[t-bltg]

I agree that it is painful to fix these things. But defining disp(x) = show(stdout, "text/plain", x) in your CI test scripts is always possible if you want to avoid using show(stdout, "text/plain", x) and make the tests pass on nightly and e.g. 1.6.5 or 1.7.2.

[xitology]

My point is that it is not a bugfix but a breaking change and should be treated as such.

[t-bltg]

My point is that it is not a bugfix but a breaking change and should be treated as such.

I'd say it's debatable whether it is a bug fix or a breaking change.

[mkitti]

Perhaps we should add something in Compat.jl?