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

Add support for imports field in standalone files #1789

Merged
merged 5 commits into from
Mar 14, 2023
Merged

Conversation

lionel-
Copy link
Member

@lionel- lionel- commented Mar 7, 2023

This will pick up the new imports: field added in r-lib/rlang#1589 and ensure all dependencies are with sufficient versions are included in Imports.

@lionel- lionel- force-pushed the standalone-imports branch from 04e878a to 4eda340 Compare March 7, 2023 14:56
@lionel- lionel- requested review from hadley and jennybc March 7, 2023 15:02
@lionel-
Copy link
Member Author

lionel- commented Mar 7, 2023

Looks like the snapshot failure is from the latest commit to main.

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.

It feels like this is moving towards what we normally accomplish with DESCRIPTION. I end up wondering if we should lean into that, so we can use existing tooling for parsing this info.

@jennybc
Copy link
Member

jennybc commented Mar 7, 2023

I think what I'm literally suggesting is: perhaps we should use DCF for this frontmatter. That would make it easier to get the Imports info into desc and let it do the parsing, validating, etc.

@@ -42,10 +57,24 @@ use_standalone <- function(repo_spec, file = NULL, ref = NULL, host = NULL) {
write_over(proj_path(dest_path), lines, overwrite = TRUE)

dependencies <- standalone_dependencies(lines, path)
for (dependency in dependencies) {

for (dependency in dependencies$deps) {
use_standalone(repo_spec, dependency)
Copy link
Member

Choose a reason for hiding this comment

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

I'm noticing that we have a pre-existing problem with not passing along host.

Should ref propagate as well? I would think so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now fixed.

@lionel-
Copy link
Member Author

lionel- commented Mar 7, 2023

I think what I'm literally suggesting is: perhaps we should use DCF for this frontmatter. That would make it easier to get the Imports info into desc and let it do the parsing, validating, etc.

Do we have any non-internal tools for parsing versions etc? I tried to find some but my research wasn't fruitful. I was planning to use DCF like this because I initially thought the desc package would help here, it looked like this:

imports <- paste0("Imports: ", paste0(imports, collpase = ", "))
desc <- desc::desc(text = imports)

But this didn't help much because I still needed to parse the versions after a get_deps(). I think what I take from this is that the hard part isn't in ingesting yaml or dcf formatting but in the parsing of the key values themselves.

Worth noting that DCF is less structured than yaml and doesn't have a standard way of specifying lists/vectors, sometimes it's just comma-separated text, sometimes it's in R code.

@jennybc
Copy link
Member

jennybc commented Mar 7, 2023

I think this looks pretty promising:

library(desc)

imports <- c(
  "Imports:",
  "    cli (>= 3.0.1),",
  "    clipr (>= 0.3.0),",
  "    crayon"
)

d <- desc(text = imports)
d$get_deps()
#>      type package  version
#> 1 Imports     cli >= 3.0.1
#> 2 Imports   clipr >= 0.3.0
#> 3 Imports  crayon        *

It seems desc is happy to operate on a DESCRIPTION fragment.

Update: or is this what you meant? I.e. that we still have ">= 3.0.1" as the version.

@lionel-
Copy link
Member Author

lionel- commented Mar 7, 2023

That's what I thought at first but I still needed to extract the comparison and the version.

@lionel- lionel- force-pushed the standalone-imports branch from 4eda340 to e1976a2 Compare March 7, 2023 17:00
@lionel-
Copy link
Member Author

lionel- commented Mar 8, 2023

I think the question of using DCF vs yaml is mosty orthogonal to this PR but here is a summary of my views:

  • From the user point of view, the advantage of DCF is the familiar syntax for the fields that happen to be the same in meaning and syntax, e.g. Imports:. The advantage of yaml is to be consistent with other configuration headers such as Rmd or quarto.

  • From the developer point of view, DCF has the disadvantage of not having list syntax. This means that nested lists can't easily be parsed unless they are supplied in another language for a second round of parsing, e.g. R code.

  • For the cases where existing DCF/Description tooling can be leveraged, it's easy to assemble a DCF field since ambiguities only arise for extracting list elements, not assembling them. Ideally though, such tooling would have variants that accept character vectors of list elements so that they can be flexibly used with different formats.

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.

I added some documentation: basically taking info implicit in various parts of the PR and exposing it in the help.

It still seems like a lot of bespoke work to add this feature, but I guess it's just a fussy task.

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