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

refactor based on overloading Base.show. #163

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

korsbo
Copy link
Owner

@korsbo korsbo commented Apr 12, 2021

This is a competing refactorisation to #161

It overloads Base.show with MIME"text/latexify". This could allow others to extend latexify's functionality without ever even loading Latexify.jl itself.

It's based on suggestions by @gustaphe #110

@korsbo korsbo changed the title Add refactor prototype overloading show. refactor based on overloading Base.show. Apr 12, 2021
@korsbo
Copy link
Owner Author

korsbo commented Apr 12, 2021

One potential problem with this approach is that Latexify.jl does not own Base.show and can thus not precompile it.


show(io::IO, ::MIME"text/latexify", x) = print(io, x)

compare_precedence(a, b) = Base.operator_precedence(a) > Base.operator_precedence(b)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure this will do the right thing for things like 1 - (2 + 3 + 4) -- here the precedence is equal, but compare_precedences will return false. Perhaps that is solved by changing the > to >=, but will it then do the right thing for 1 - (2 - 3 - 4)?

I guess the show method for :- could give :+ as precedence for left and :- for right.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this will not always do the right thing

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not even convinced that operator_precedence is reliably good for latex purposes. I think one might find plenty of cases where code would need parentheses while latex would not.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there exists such an ordering, it's pretty simple to implement your own operator_precedence. Otherwise one might need individual compare_precedence(::Val, ::Val) functions.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

individual comparisons is pretty much what I have been doing before (not saying that this it the way to go). It turned out not to be needed too frequently though. - and ^ turned out to be the ones which needed the most care in figureing out where parentheses are needed.

@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #163 (1b3e25e) into master (9416101) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #163   +/-   ##
=======================================
  Coverage   60.21%   60.21%           
=======================================
  Files          23       23           
  Lines         734      734           
=======================================
  Hits          442      442           
  Misses        292      292           

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 9416101...1b3e25e. Read the comment docs.

@gustaphe
Copy link
Collaborator

One potential problem with this approach is that Latexify.jl does not own Base.show and can thus not precompile it.

I haven't worked with precompilation, but don't you just need to own the methods? precompile(show, (IO, MIME"text/latex", Int64)) should be uncontroversial. Or am I misunderstanding?

@korsbo
Copy link
Owner Author

korsbo commented Apr 13, 2021

One advantage of #161 over this approach is that it would be possible to let the list of matching functions be passed in as a kwarg or something that is specific to a single call.

Here, all extensions are global and the overloads created in one package might bleed over and affect others. If a user wanted to, for example, strip away all macro call annotations they could overload show(... x::Operation{Val{:macrocall}, T}) where T = show(io, mime, args(x)[end]). That would work nicely but it would affect the latexification done in other packages too. Having users choose between piracy and simply not being able to do what they want is not ideal.

In #161 it might be possible to let extending packages do what they want while keeping the effects confined to only apply when the top-level call takes their own types. There could then be two modes of extension - local or global (for things like UnitfulLatexify.jl). Here, only global is possible.

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.

2 participants