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

Simplify handling of Suggested dependencies #1394

Merged
merged 15 commits into from
Jun 7, 2023
Merged

Simplify handling of Suggested dependencies #1394

merged 15 commits into from
Jun 7, 2023

Conversation

hadley
Copy link
Member

@hadley hadley commented Jun 5, 2023

  • Top-level DESCRIPTION is now treated the same way regardless of whether it's a "Package" or not.
  • settings$package.dependency.fields() no longer affects transitive dependencies.
  • Document how "development dependencies" are found in dependencies()

* So a top-level `DESCRIPTION` is treated the same way regardless of whether it's a "Package" description or not.

* Document how "development dependencies" are found in `dependencies()`

* Impute a roxygen2/devtools dependency if the package has a `RoxygenNote` field
@hadley hadley requested a review from kevinushey June 5, 2023 22:08
R/dependencies.R Outdated Show resolved Hide resolved
@hadley hadley marked this pull request as draft June 5, 2023 22:13
@hadley
Copy link
Member Author

hadley commented Jun 6, 2023

To get the tests to pass, I kept pulling the string and it kept getting longer and longer. I think I see a simpler/cleaner approach so no need to review until I've tried that.

@@ -195,14 +210,6 @@ test_that("Suggest dependencies are ignored by default", {
expect_false(renv_package_installed("egg"))
})

test_that("Suggest dependencies are used when requested", {
Copy link
Member Author

@hadley hadley Jun 6, 2023

Choose a reason for hiding this comment

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

Moved to install.R which is a better location.

@hadley hadley marked this pull request as ready for review June 6, 2023 18:55
@hadley
Copy link
Member Author

hadley commented Jun 6, 2023

@kevinushey It's worth taking another look at this now, since I've also slightly changed the meaning of settings$package.dependency.fields().

NEWS.md Show resolved Hide resolved
R/dependencies.R Outdated Show resolved Hide resolved
renv_dependencies_list(
path,
extract_chr(matches, 2L),
extract_chr(matches, 3L),
extract_chr(matches, 4L),
dev
dev = field == "Suggests"
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW I think the reason I had this was to avoid issues with DESCRIPTION files located in sub-directories within a project, but that imply separate considerations (e.g. should we really be parsing those for dependencies at all from the top-level?)

R/package.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

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

LGTM!

@hadley hadley merged commit 334511e into main Jun 7, 2023
@hadley hadley deleted the suggested-deps branch June 7, 2023 16:47
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