Remove {prettycode} dependency in favour of `cli::code_highlight()`

[wurli]

As discussed in #486.

The only thing to note is that this changes the behaviour of the constructive_pretty global option. Previously this defaulted to TRUE with an additional requirement that {prettycode} be installed, but now it defaults to FALSE and the user has to manually override it. IMO it's nice to have the option to use highlighting, but there would be some hidden costs to turning it on by default:

• {cli}'s hyperlinks might be annoying if people accidentally click them when trying to copy code from the console

• Highlighting will likely look quite different to the user's editor theme, so turning it on by default would naturally introduce some visual dissonance and potentially make {constructive} feel subtly worse to use

• Some users only find a very small number of themes easy to read, e.g. due to colourblindness, so {cli}'s application of colour might not be an improvement for everyone

Thanks!

[aviator-app[bot]]

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes. Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.

---

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.

[moodymudskipper]

Thanks! I think we need the highlighting by default, most users don't read the doc or use options, they just use construct() instead of dput(), so turning it off by default will worsen the experience for most and thy won't understand why constructive look worse than before.

• However we should use a code_theme arg where we had style in print.
• style need to be removed from the args but caught from the dots in the function so we can provide a warning that it doesn't work anymore (or better, we could even fetch the colors from style and change them to a code_theme, so we can remap silently).
• We didn't provide an option to set the style but we might provide one to set the code_theme, this would replace the existing option. In the doc we mention that the empty list removes all highlighting. The existing option can be removed from the doc, but we still consider it for backward compatibility (without warning).

If it's a hassle I can take those, it's important for me that we make it as a little of a breaking change as possible.

[wurli]

Heya, thanks for the review!

About whether to add highlighting by default, I'm happy to make this change if you think it's definitely the way to go. I was thinking that it's probably a fairly small minority of users who are using {prettycode} already, so while turning it on as a default would have the benefit of not removing colours for a small number of users, the potential downside would be that it could introduce an unwanted feature for the majority. That said, many people might also like it a lot. What do you think, best to turn it on for everyone and maybe advertise how to turn it off in NEWS.md?

About the style argument, thanks for the heads up, I missed that it was exposed to the user through print(). Happy to add a deprecation message and code_theme as a replacement. After a quick look at prettycode::default_style() I think a translation feature would probably be more trouble than it's worth.

As far as a global option to set code_theme goes, I think that cli.code_theme is probably enough control, but it should probably be advertised in the docs. We could add a constructive.code_theme option to override cli.code_theme, but I can't imagine why anyone would ever need this.

In the doc we mention that the empty list removes all highlighting.

Do you mind pointing me to where this is please? I had a look but I couldn't find the bit you're referring to...

[moodymudskipper]

Fair point. I thought most had prettycode installed, it is or was used by styler, but styler might not be as common as I expect. In any case I believe the code highlighting is a great feature and should be the default.

What do you think, best to turn it on for everyone and maybe advertise how to turn it off in NEWS.md?

Yes

After a quick look at prettycode::default_style() I think a translation feature would probably be more trouble than it's worth.

It's ok to skip it, I don't believe people used it other than maybe interactively, and it's not recorded in snapshots.

As far as a global option to set code_theme goes, I think that cli.code_theme is probably enough control, but it should probably be advertised in the docs. We could add a constructive.code_theme option to override cli.code_theme, but I can't imagine why anyone would ever need this.

You are correct, no need to have a local option for this

Do you mind pointing me to where this is please? I had a look but I couldn't find the bit you're referring to...

I wasn't clear, I meant that we "would" mention, if we implemented this option

So there's much less to change after all :D.

[wurli]

Hey @moodymudskipper, I've made the suggested changes now. Things to note:

• I've imported rlang's standalone-lifecycle.R in favour of actually using {lifecycle} or rolling my own solution
• I've also added documentation for print.constructive_code(). This somehow didn't feel like a great place to be adding user documentation, but short of adding a dedicated man page or vignette it seemed like the least worst option.

Thanks!

[wurli]

Oops, fixing R CMD check now...

[moodymudskipper]

We have a problem with cli :(

x <- paste0("fun('", strrep("-", 1000), "')")
prettycode::highlight(x)
#> [1] "fun('----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------')"
cli::code_highlight(x)
#> [1] "fun([1000 chars quoted with '''])"

^{Created on 2024-09-30 with reprex v2.1.0}

I had pushed a fix to prettycode : https://github.com/r-lib/prettycode/pull/21 that in fact hasn't been pushed to CRAN, and maybe won't ever be as it looks prettycode might as well retire (Gabor maintains both cli and prettycode)

I opened this: https://github.com/r-lib/cli/issues/726

Even if it doesn't change anything for CRAN versions I'd like cli to handle this on dev before I merge this

[wurli]

Good catch! Hopefully will be fixed in cli soon.

[moodymudskipper]

I proposed a PR there, it's better than what I had pushed to prettycode so hopefully this is all good news. https://github.com/r-lib/cli/pull/727

Thanks for your patience!

[wurli]

No rush on my end 🙂

[aviator-app[bot]]

Free usage limit reached: 15 users this month.To add more users, upgrade to the Pro plan.

[moodymudskipper]

https://github.com/r-lib/cli/pull/727 was finally merged, when next version of cli is pushed to CRAN we can merge this. We're now on 3.6.5

[moodymudskipper]

cli 3.6.5 is on CRAN, 2 conflicts remain in NEWS and in construct_diff() where no formatting should be applies (was applied by mistake before and the error was translated in this PR). @wurli if you can do it that's great, otherwise I'll take care of it when I get back to constructive in a week or 2.

[moodymudskipper]

Thanks!

[codecov[bot]]

Codecov Report

:x: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 73.17%. Comparing base (783ebb6) to head (0dc1b8f). :warning: Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
R/construct.R	33.33%	4 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #488      +/-   ##
==========================================
- Coverage   73.20%   73.17%   -0.04%     
==========================================
  Files         160      160              
  Lines        5688     5689       +1     
==========================================
- Hits         4164     4163       -1     
- Misses       1524     1526       +2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.