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

accept Vector of highlighters. #169

Open
oscardssmith opened this issue May 5, 2022 · 8 comments
Open

accept Vector of highlighters. #169

oscardssmith opened this issue May 5, 2022 · 8 comments

Comments

@oscardssmith
Copy link

no reason not to.

@ronisbr
Copy link
Owner

ronisbr commented May 6, 2022

Hi @oscardssmith !

Previously, we supported a vector as the highlighters. To use this, I need to make the option a Union between Highlighter, Tuple{Highlighter}, and Vector{Highlighter}. Those options were adding a significant amount of time during the package loading (due to the precompilation). Hence, I decided to remove the vector to reduce it. However, this scenario was tested during Julia 1.3 or 1.4 (can't remember correctly). Maybe it is improved and we can return this option. I will investigate.

@oscardssmith
Copy link
Author

What about having a method that takes a Vector{Highlighter} and just calls Tuple on it, and passes it to the main function? That way the Vector one will be type unstable (which IMO is probably fine), and the complicated code won't be any more complicated. Also it does seem to me like it might make more sense to use the Vector internally since Tuple will start failing pretty badly if you want to have more than 30 or so highlighters (not sure how common this is).

@ronisbr
Copy link
Owner

ronisbr commented May 6, 2022

Hum, I did not know about this limitation in the size of the tuples. I agree with your suggestion, I just need to check the best way to do this because we have three backends with different Highlighters. Hence, I am not sure what is the best position to add this conversion function. Thanks!

@oscardssmith
Copy link
Author

it's not a hard limitation, but large tuples have a large impact on compile time.

@ronisbr
Copy link
Owner

ronisbr commented May 6, 2022

Perfect! Understood. Thanks!

@ronisbr
Copy link
Owner

ronisbr commented Aug 20, 2022

Hi @oscardssmith ,

Just an update. I tested by changing everything to Vectors instead of Tuples. However, the time to print the first table almost doubled! I have no idea why and I could not fix it. I am still investigating.

@oscardssmith
Copy link
Author

interesting. I would have expected the opposite. can you make it a pr so I can test also?

@ronisbr
Copy link
Owner

ronisbr commented Aug 20, 2022

Sure! When I have some time I will clean-up the code and submit to a branch.

Thank you very much!

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

No branches or pull requests

2 participants