tweak width of output

[olivroy]

follow-up to #1187

The +1 seems to cause rendering issues It seems like it was added in https://github.com/r-lib/styler/pull/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.

[github-actions[bot]]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 208d9c7abb4eb44436695a68a19ea979c99ce921 is merged into main:

•   :ballot_box_with_check:cache_applying: 147ms -> 149ms [-1.52%, +4.79%]
•   :ballot_box_with_check:cache_recording: 510ms -> 508ms [-1.33%, +0.27%]
•   :ballot_box_with_check:without_cache: 975ms -> 978ms [-0.48%, +0.98%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[lorenzwalthert]

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 test-public-api-*.R

[lorenzwalthert]

FYI @olivroy I am finish ing the release 1.10.3, so if you want this and https://github.com/r-lib/styler/pull/1187 to be released as part of that (no idea when the next release comes), please make the changes requested in the next 2 days.

[olivroy]

Thanks! I will be able to make it tomorrow

[olivroy]

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 test-public-api-*.R

Seems like width of output is more difficult to test programatically with snapshots. according to codecov, the lines I changed are already covered..

[lorenzwalthert]

Can you explain what are the hurdles to add a snapshot test? We can set the width of the output programmatically with R option (options(width = 80) using {withr} with local_options() or similar to ensure consistent width.

Even when already covered, current tests seem insufficient to detect regressions like the one you introduced.

[olivroy]

Can you explain what are the hurdles to add a snapshot test? We can set the width of the output programmatically with R option (options(width = 80) using {withr} with local_options() or similar to ensure consistent width.

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. https://github.com/rstudio/rstudio/issues/13869

[olivroy]

See the difference how cli markup is rendered vs regular one (I am using dev usethis, which uses cli internally now)

With cli, the message is displayed on 3 lines, while the regular cat() message is displayed on 2 lines. This is to avoid breaking links or markup.

[lorenzwalthert]

With the release candidate,I get

Note that the checks are vertically aligned in the release candidate. Your snapshot test should show that too (but don't I think). So I think we need to work on the implementation of your formatting to more closely mimic the release version.

[olivroy]

Ok. I don't really know how to make console + RStudio agree. I will send a PR to revert #1187. Sorry for the noise

[lorenzwalthert]

Ok. I don't really know how to make console + RStudio agree.

I get this with the console, so console and RStudio look similar tome?

[olivroy]

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!

[lorenzwalthert]

The problem is that if you have a path that is wider than the width, all checks marks would go to the next line..

I agree, but that problem already exists currently, so I would not consider that blocking a merge of this PR. here both release candidate and your PR

enough wide (kind of):

not enough