devtools icon indicating copy to clipboard operation
devtools copied to clipboard

use_tidy_dependencies

Open ateucher opened this issue 1 year ago • 11 comments

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 avatar Mar 07 '23 00:03 ateucher

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.

ateucher avatar Mar 07 '23 06:03 ateucher

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.

jennybc avatar Mar 07 '23 18:03 jennybc

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")) ...)?

ateucher avatar Mar 07 '23 18:03 ateucher

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.

jennybc avatar Mar 07 '23 19:03 jennybc

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

ateucher avatar Mar 07 '23 19:03 ateucher

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.

jennybc avatar Mar 07 '23 21:03 jennybc

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?

ateucher avatar Mar 08 '23 16:03 ateucher

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.

jennybc avatar Mar 08 '23 17:03 jennybc

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

ateucher avatar Mar 08 '23 17:03 ateucher

@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?

jennybc avatar Mar 08 '23 22:03 jennybc

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.

hadley avatar Mar 08 '23 23:03 hadley