Skip to content
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 newline #763

Closed
wants to merge 2 commits into from
Closed

display newline #763

wants to merge 2 commits into from

Conversation

t-bltg
Copy link

@t-bltg t-bltg commented Jan 18, 2022

Per JuliaLang/julia#43787 and JuliaLang/julia#43787 (comment).

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

@fredrikekre
Copy link
Member

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

@codecov
Copy link

codecov bot commented Jan 18, 2022

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%) ⬇️

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
Copy link

mkitti commented Jan 18, 2022

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

@t-bltg
Copy link
Author

t-bltg commented Jan 18, 2022

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
Copy link

mkitti commented Jan 18, 2022

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

@t-bltg
Copy link
Author

t-bltg commented Jan 18, 2022

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
Copy link

mkitti commented Jan 18, 2022

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.

src/Compat.jl Show resolved Hide resolved
@mkitti
Copy link

mkitti commented Jan 18, 2022

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
Copy link
Author

t-bltg commented Jan 18, 2022

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 JuliaLang/julia#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
Copy link
Author

t-bltg commented Jan 18, 2022

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants