-
Notifications
You must be signed in to change notification settings - Fork 71
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
tweak width of output #1189
tweak width of output #1189
Conversation
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 208d9c7 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
Thanks. Seems not to slow down things. 👍 also, can you add a snapshot test for this output? Seems like we don’t have good coverage of that currently. I think there are already snapshot tests in the |
Thanks! I will be able to make it tomorrow |
Seems like width of output is more difficult to test programatically with snapshots. according to codecov, the lines I changed are already covered.. |
Can you explain what are the hurdles to add a snapshot test? We can set the width of the output programmatically with R option ( Even when already covered, current tests seem insufficient to detect regressions like the one you introduced. |
It is just that RStudio renders inline markup peculiarly. Before RStudio 2023.12, long output could be truncated, so they introduced a new way to render console output that seems to break lines more often than usual, but that doesn't really show in snapshot tests.. Only interactive testing seems to work for this. rstudio/rstudio#13869 |
Ok. I don't really know how to make console + RStudio agree. |
I agree my fix was incorrect. The problem is that if you have a path that is wider than the width, all checks marks would go to the next line.. But it is better to revert I guess, not much added value! |
follow-up to #1187
The +1 seems to cause rendering issues
It seems like it was added in #165
I know that cli exports
cli::ansi_nchar()
, but it will probably slow down a little bit the execution to use this.while on main currently, I had
Sorry I didn't test this more extensively in the first place.