JuliaLang / Compat.jl

Compatibility across Julia versions
Other
144 stars 117 forks source link

`display` newline #763

Closed t-bltg closed 9 months ago

t-bltg commented 2 years ago

Per https://github.com/JuliaLang/julia/pull/43787 and https://github.com/JuliaLang/julia/pull/43787#issuecomment-1013898482.

@mkitti, since I'm unfamiliar with Compat.jl, I hope this is correct ?

fredrikekre commented 2 years ago

It would have to be a separate function (Compat.display) I believe.

codecov[bot] commented 2 years ago

Codecov Report

Merging #763 (4f61b59) into master (a2de107) will decrease coverage by 0.29%. The diff coverage is 40.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #763      +/-   ##
==========================================
- Coverage   78.90%   78.61%   -0.30%     
==========================================
  Files           4        4              
  Lines         659      664       +5     
==========================================
+ Hits          520      522       +2     
- Misses        139      142       +3     
Impacted Files Coverage Δ
src/Compat.jl 78.59% <40.00%> (-0.34%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a2de107...4f61b59. Read the comment docs.

mkitti commented 2 years ago

Perhaps alternate TextDisplay might make sense rather than creating an alternate display?

t-bltg commented 2 years ago

My second implementation defining Compat.display doesn't seem to work.

ERROR: MethodError: no method matching display(::TextDisplay, ::Int64)
You may have intended to import Base.display

I would have to define all the display methods in Compat ?

Fixed.

mkitti commented 2 years ago

I would implement a CompatTextDisplay <: AbstractDisplay. You could then push! that into Base.Multimedia.displays to get compatible behavior.

t-bltg commented 2 years ago

I would implement a CompatTextDisplay <: AbstractDisplay. You could then push! that into Base.Multimedia.displays to get compatible behavior.

@fredrikekre suggested that we shouldn't override the default behavior and use Compat.display(...) instead. Pushing to Base.Multimedia.displays will affect the default display, no ?

mkitti commented 2 years ago

The approaches are not mutually exclusive although the trick is in creating separate namespaces. You might need to create submodules to contain one or both.

The advantage of making a separate CompatTextDisplay is that you will be able to influence how Base.display works in code that you do not have access to. Perhaps you are using another module that uses display, and you do not want to have to vendor or modify that package directly.

Just to clear, Compat.jl should not necessarily push! CompatTextDisplay automatically on __init__. Rather this is something the end application developer might do.

mkitti commented 2 years ago

I just read this more carefully, and I see that now that you are mainly trying to make the new behavior available in older Julia as opposed to making the old behavior available in new Julia.

t-bltg commented 2 years ago

I just read this more carefully, and I see that now that you are mainly trying to make the new behavior available in older Julia as opposed to making the old behavior available in new Julia.

Exactly, if I'm not misunderstanding the word latest, quoting from README.md:

the Compat package provides a macro that lets you use the latest syntax in a backwards-compatible way.

Taking the example of @xitology, in https://github.com/JuliaLang/julia/pull/43787, he could now write: Compat.display(...) in his test scripts, with the same behavior on 1.6 or 1.7 and 1.8.

t-bltg commented 2 years ago

The advantage of making a separate CompatTextDisplay is that you will be able to influence how Base.display works in code that you do not have access to. Perhaps you are using another module that uses display, and you do not want to have to vendor or modify that package directly.

That might be handy.

Just to clear, Compat.jl should not necessarily push! CompatTextDisplay automatically on __init__. Rather this is something the end application developer might do.

I'd be happy to review a PR, my implementation is not necessarily the one we need, and I don't have a clear vision of what you propose as an alternative.

Or you can also push in this branch!

LilithHafner commented 9 months ago

I don't think we need this anymore. IIUC breakage was all transient in scripts and the window for adding tooling with a significant impact on that has closed.