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

Replace crayon by cli + address some TODOs to add some color #4170

Merged
merged 4 commits into from
Jan 21, 2025

Conversation

olivroy
Copy link
Contributor

@olivroy olivroy commented Dec 12, 2024

Also removed RdMacros since no longer used. (also no longer recommended by pkgdown..

The end goal is to have clickable links in output.

info (to ignore for now)

With a change I have locally (not included here), I am able to acheive the following for the stack trace. However, there is a slowdown. So, I will look into it.

image

@gadenbuie
Copy link
Member

However, there is a slowdown. So, I will look into it.

Are there any performance changes in what's currently in the PR?

@olivroy
Copy link
Contributor Author

olivroy commented Jan 17, 2025

Sorry, I should have been clearer. I didn't include the change in question in this PR. I was just storing this unrelated info here to make sure I'd find it when I needed it.

@gadenbuie
Copy link
Member

That makes sense; it's also how I interpreted your notes. In other words, since you had looked at the speed, it'd be helpful to know if there are any performance changes in the PR as it is now.

Also, if you're still working on this and aren't ready for a review, would you mind changing the PR to a draft to avoid confusion?

@olivroy
Copy link
Contributor Author

olivroy commented Jan 20, 2025

No, I am done with this PR! I observed decreased performance with some other changes I didn't include in the PR (that I may send as a follow-up if I figure out how to limit decreased performance)

Here are some bench marks for this PR.

# Simple color

# cli more performant!

bench::mark(
  x1 = crayon::silver("xx"),
  x2 = cli::col_silver("xx"),
  check = FALSE
)
#> # A tibble: 2 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 x1           68.4µs    147µs     5313.   172.4KB     2.03
#> 2 x2           22.7µs     25µs    30405.    76.4KB     3.04


# Bold: no performance lost
bench::mark(
  x1 = crayon::blue$bold("xx"),
  x2 = cli::style_bold(cli::col_blue("xx")),
  check = FALSE
)
#> # A tibble: 2 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 x1           73.7µs  117.8µs     8392.    30.3KB     6.21
#> 2 x2           42.7µs   70.9µs    14126.      44KB     4.09

# cat vs cli::cat_bullet().
# slight performance decrease
# but still fast (26 microseconds vs 336 microseconds)
bench::mark(
  x1 = cat("* Success\n"),
  x2 =  cli::cat_bullet("Success", bullet = "tick", bullet_col = "green")
)

#> # A tibble: 2 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 x1           18.3µs   22.5µs    38086.    11.5KB     3.81
#> 2 x2            336µs    421µs     1893.    27.2KB     4.14

Created on 2025-01-20 with reprex v2.1.1

Copy link
Member

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @olivroy and thanks for including the extra benchmarking info!

@gadenbuie gadenbuie merged commit 7642fc8 into rstudio:main Jan 21, 2025
12 checks passed
@olivroy olivroy deleted the stack-click branch January 21, 2025 16:57
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

Successfully merging this pull request may close these issues.

2 participants