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

use_tidy_dependencies #2503

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

use_tidy_dependencies #2503

wants to merge 21 commits into from

Conversation

ateucher
Copy link
Contributor

@ateucher ateucher commented Mar 7, 2023

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

Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

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

Looking good. I made a few comments, mostly at the level of nitpicks.

Once this advances one more click, we can tag Hadley in to discuss the number of dependencies.

On one hand, it would be nice to stay below the threshhold. But on the other hand, given the nature of this package, which is similar to that of the tidyverse meta-package, maybe this just isn't a sustainable position and we should admit it's a meta-package instead of doing weird gymnastics.

R/release.R Show resolved Hide resolved
NAMESPACE Show resolved Hide resolved
R/build-manual.R Outdated Show resolved Hide resolved
R/check-devtools.R Outdated Show resolved Hide resolved
R/check-win.R Outdated Show resolved Hide resolved
@ateucher
Copy link
Contributor Author

ateucher commented Mar 7, 2023

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
Copy link
Member

jennybc commented Mar 7, 2023

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
Copy link
Contributor Author

ateucher commented Mar 7, 2023

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
Copy link
Member

jennybc commented Mar 7, 2023

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
Copy link
Contributor Author

ateucher commented Mar 7, 2023

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
Copy link
Member

jennybc commented Mar 7, 2023

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
Copy link
Contributor Author

ateucher commented Mar 8, 2023

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
Copy link
Member

jennybc commented Mar 8, 2023

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
Copy link
Contributor Author

ateucher commented Mar 8, 2023

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

@jennybc
Copy link
Member

jennybc commented Mar 8, 2023

@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 jennybc requested a review from hadley March 8, 2023 22:55
NEWS.md Outdated Show resolved Hide resolved
@hadley
Copy link
Member

hadley commented Mar 8, 2023

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.

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.

4 participants