r-lib / devtools

Tools to make an R developer's life easier
https://devtools.r-lib.org
Other
2.37k stars 755 forks source link

use_tidy_dependencies #2503

Open ateucher opened 1 year ago

ateucher commented 1 year ago

This uses use_tidy_dependencies(), which most consequentially adds glue to Imports, and adds brings in import-standalone-purrr.R

To start to make use of these I've replaced most instances of paste0() with glue(). I've also used the purrr-like functionality to remove the DIY compact(), replaced vcapply() with map_chr() and vapply() calls with their map_ equivalent.

The addition of glue takes the number of non-default packages to 21 which triggers a NOTE about excessive dependencies. Although not used, I see that profvis, miniUI, and bench are included in Imports - presumably so the user has them installed so they can use them without fuss?

Looking at pak::pkg_deps_explain("devtools", "cli"), I wonder if cli would be a good candidate to move to Suggests and use cli:: throughout? It has many dependency pathways to ensure that it will always be installed, and it is easy to qualify with its namespace (and appears to be mostly called that way already in devtools).

Address part of #2501

ateucher commented 1 year ago

Thanks @jennybc! I think I've addressed your comments and suggestions.

Regarding the number of dependencies, I do think it feels a bit messy to sort of obfuscate by moving packages to Suggests to get around an arbitrary threshold. If there's a way to keep packages in Imports that should be there and not feel too much pain with CRAN submissions I think that would be ideal.

jennybc commented 1 year ago

Assuming we give up on playing games with the dependencies, here's how it's handled in the tidyverse package:

https://github.com/tidyverse/tidyverse/blob/main/cran-comments.md

The same rationale applies here as well.

## R CMD check results

0 errors | 0 warnings | 1 notes

> checking package dependencies ... NOTE
  Imports includes 29 non-default packages.
  Importing from so many packages makes the package vulnerable to any of
  them becoming unavailable.  Move as many as possible to Suggests and
  use conditionally.

This is a false positive - the majority of packages are maintained by me or my team, so there's little risk of them becoming unavailable.
ateucher commented 1 year ago

Nice and simple, I like it. If we decide to go that route, should I look in Suggests and move to Imports those packages that are used unconditionally (i.e., without if (!requireNamespace("pkg")) ...)?

jennybc commented 1 year ago

If we decide to go that route, should I look in Suggests and move to Imports those packages that are used unconditionally (i.e., without if (!requireNamespace("pkg")) ...)?

That seems sensible. A good first step would be to determine which packages that applies to, if any, for concreteness.

ateucher commented 1 year ago

There are three packages in Suggests that are called unconditionally in code: callr, httr, and rstudioapi.

All have quite strong indirect dependency relationships but I think are probably worth moving into Imports as they are widely used and pretty critical:

library(pak)

pkg_deps_explain("devtools", deps = "callr")
#> devtools -> pkgbuild -> callr
#> devtools -> pkgdown -> callr
#> devtools -> rcmdcheck -> callr
#> devtools -> rcmdcheck -> pkgbuild -> callr
#> devtools -> testthat -> callr
pkg_deps_explain("devtools", deps = "httr")
#> devtools -> pkgdown -> httr
pkg_deps_explain("devtools", deps = "rstudioapi")
#> devtools -> usethis -> gert -> rstudioapi
#> devtools -> usethis -> rstudioapi

Created on 2023-03-07 with reprex v2.0.2

jennybc commented 1 year ago

Yes, so these seem to be the packages that were sort of artificially moved to Suggests in #2411 to avoid the NOTE. I think putting them back in Imports brings this PR to an internally consistent state.

ateucher commented 1 year ago

I think I have two final question @jennybc:

  1. Should I run use_latest_dependencies() or leave as is? I set glue to be the current CRAN version but haven't touched the others.
  2. I think I should update NEWS.md with this changes, even though they're not really user-facing?
jennybc commented 1 year ago

Should I run use_latest_dependencies() or leave as is? I set glue to be the current CRAN version but haven't touched the others.

Yes, that seems the most appropriate thing to do in this PR where we embrace the meta-package nature of devtools.

I think I should update NEWS.md with this changes, even though they're not really user-facing?

I'm not entirely sure what we can say that's user-facing, but sure go ahead of add such a bullet and we can decide how it feels.

ateucher commented 1 year ago

Ok done - I added a few bullets to NEWS.md. Let me know if it's too much inside baseball!

jennybc commented 1 year ago

@hadley Would you like to offer an opinion?

@ateucher and I both think it makes sense to admit that devtools is a meta-package, similar to tidyverse. And, therefore, we should stop doing weird gymnastics to keep the dependencies below 20 and, instead, just do what makes sense.

This was triggered by doing use_tidy_dependencies() and, specifically, adding glue.

But I do think it would be nice to be able to use glue internally and to put callr, httr, and rstudioapi back in Imports which is where they really belong.

So I guess I have two questions:

Are you on board with this? Or at least neutral?

If so, do you agree that we should also put minimum versions to CRAN versions, similar to tidyverse?

hadley commented 1 year ago

If you're willing to argue with CRAN about this (if needed, which it probably won't be), go for it.

And yes, I do think you should bump all the versions.