Skip to content

Conversation

@olivroy
Copy link
Collaborator

@olivroy olivroy commented May 30, 2023

Summary

This PR aims to take advantage of cli inline markup and rlang utils functions (i.e. rlang::check_installed() as well as using dplyr::tibble() in the code and documentation.
Improving syntax of some error messages and improve speed of tests by using expect_named/length/match/in()

The failing tests on my Windows machine are related to file creation /saving.

Checklist

Edit: sorry for the large PR. At first, I wanted to keep it small, but ended up making changes in all places I thought were relevant for consistency and to avoid many follow-up PRs)

olivroy added 2 commits May 30, 2023 10:12
Use `rlang::check_installed()` instead of `requireNamespace()`
Use `anyNA()`
Use `dplyr::pick`
@CLAassistant
Copy link

CLAassistant commented May 30, 2023

CLA assistant check
All committers have signed the CLA.

@olivroy olivroy changed the title Various code update Improve suggested packages handling + other code update Aug 17, 2023
@rich-iannone
Copy link
Member

Sorry for not commenting sooner but I do like all the work you've put into this!

I have just one change request at the moment, and addressing that would get this PR closer to merging/completion. Would it be possible to revert all of the tibble::tibble changes back to dplyr::tibble? I know this doesn't make much sense, but I'm hoping to remove the tibble dependency at a later date (I use only a few tibble functions) and I know that dplyr will always re-export the tibble() function.

@olivroy
Copy link
Collaborator Author

olivroy commented Sep 22, 2023

@rich-iannone Thank you!

I have added a few more package handling changes + removed tibble::tibble in tests. + modernized a little the test suite.

Edit: I have looked into replacing tibble::enframe() with dplyr::tibble() but it was 10x slower (30microseconds vs 300 microseconds)

Edit 2: I also took a look at the tests. I know it is unrelated to the messages, but it helps to reduce the clutter a little. Bonus, improves speed a little.

library(magrittr)
library(testthat)
#> 
#> Attachement du package : 'testthat'
#> Les objets suivants sont masqués depuis 'package:magrittr':
#> 
#>     equals, is_less_than, not
bench::mark(
  now = letters %>% expect_no_error(),
  before = letters %>% expect_error(regexp = NA),
  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 now         191.8µs  201.4µs    4585.   246.02KB     10.6
#> 2 before       42.5ms   43.6ms      22.1    4.43MB     12.7
bench::mark(
  now = letters %>% expect_length(26),
  before = letters %>% length() %>% expect_equal(26),
  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 now           126µs  133.4µs    6709.     56.9KB     10.3
#> 2 before         85ms   86.8ms      11.5   583.4KB     23.1
nms <- names(mtcars)
bench::mark(
  now = mtcars %>% expect_named(nms),
  before = mtcars %>% names() %>% expect_equal(nms),
  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 now         140.5µs    149µs   6177.      27.3KB     10.4
#> 2 before       87.6ms    108ms      9.30   427.9KB     13.9

Created on 2023-09-23 with reprex v2.0.2

@rich-iannone
Copy link
Member

Thank you for being so diligent with the test changes (and checking the difference in timings). This helps a lot! Let me know when you want me to review and merge; I don’t want to do it too early, as you might have a few more commits to go until you consider it finalized.

@olivroy

This comment was marked as resolved.

@olivroy
Copy link
Collaborator Author

olivroy commented Sep 24, 2023

@rich-iannone Ready for your review now.

@olivroy olivroy changed the title Improve suggested packages handling + other code update Improve suggested packages handling + testthat code update Sep 24, 2023
Copy link
Member

@rich-iannone rich-iannone left a comment

Choose a reason for hiding this comment

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

Thanks again for the huge amount of work done here! I made only a few minor changes but 99.5% of this was perfect.

@rich-iannone rich-iannone merged commit bd7330d into rstudio:master Sep 25, 2023
@olivroy olivroy deleted the doc-link branch September 25, 2023 19:42
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.

3 participants