ronisbr / PrettyTables.jl

Print data in formatted tables.
MIT License
404 stars 38 forks source link

allow LaTeXStrings #186

Closed BeastyBlacksmith closed 1 year ago

BeastyBlacksmith commented 2 years ago

As I said in https://discourse.julialang.org/t/course-about-creating-reports-with-julia/87570/16?u=beastyblacksmith LaTeXStrings is a benign dependency, that enables composition by multiple dispatch. As a user, I don't want to have the resposibility to check whether the output of package A is a valid latexstring when passing it to package B. Therefore you need to have a messanger type thats neither part of package A or B. And that is a good thing.

ronisbr commented 2 years ago

The problem is that LaTeXString is not in Julia stdlib. As I said, this can escalate quickly. What about the same problem in HTML? What should we do if another package does something similar to LaTeXString? PrettyTables is suppose to be a low level printing package that others can use.

Can you please point me a real situation that adding this dependency to PrettyTables will be almost impossible to circumvent?

@bkamins What do you think? In the past, we removed Parameters.jl, which added a similar loading time, to keep the dependencies as small as possible.

bkamins commented 2 years ago

Weighing pros and cons, my thinking is as follows:

  1. Parameters.jl is developer oriented; it can be dropped without impacting user experience.
  2. LaTeXStrings.jl is end-user oriented; it is aimed to make user's life easier

So it means that LateXStrings.jl should have a preference over Parameters.jl to be included. Now there are two considerations:

  1. Is LaTeXStrings.jl lightweight: yes it is, it is I think 3x smaller than Parameters.jl;
  2. Does LaTeXStrings.jl ensure proper maintenance: I think yes given that it has wide adoption, is a simple package (no need of changes), is maintained by @stevengj, who ensures quality.

In conclusion: if users need it I would accept it.

ronisbr commented 2 years ago

Ok! So let’s add this dependency!

@BeastyBlacksmith can you please add tests considering cells with LaTeXStrings?

ronisbr commented 2 years ago

I did some minor modifications. If the test passes, I will merge.

ronisbr commented 1 year ago

Merged! Thanks @BeastyBlacksmith !