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 use_import_from() #1377

Merged
merged 33 commits into from Mar 3, 2021
Merged

Add use_import_from() #1377

merged 33 commits into from Mar 3, 2021

Conversation

malcolmbarrett
Copy link
Collaborator

This PR adds a new function, use_import_from(), that adds @importFrom to the package doc, potentially as a general replacement for for functions like use_tibble(). It also adds pkgload to Suggests.

I made the judgment call to rename the proposed function to use_import_from() so that it was clearer that the function would not add @import, only @importFrom.

While I did not touch use_pipe() or use_tibble(), this function (or some of its internal functionality) could replace some of the boilerplate in those functions. That might be desirable if you want to start pushing use_package_doc() a bit more. Notably, since this function does not facilitate @import uses, it cannot replicate use_data_table().

Closes #1361

cc: @hadley

R/use_import_from.R Outdated Show resolved Hide resolved
R/use_import_from.R Outdated Show resolved Hide resolved
R/use_import_from.R Outdated Show resolved Hide resolved
R/use_import_from.R Outdated Show resolved Hide resolved
R/use_import_from.R Outdated Show resolved Hide resolved
R/use_import_from.R Outdated Show resolved Hide resolved
R/use_import_from.R Outdated Show resolved Hide resolved
@jennybc
Copy link
Member

jennybc commented Feb 26, 2021

usethis does several things that are usually followed by a call to load_all() (e.g. use_data(), use_namespace()).

Implicitly I've sort of thought this: "modify the source package = usethis's job, other package workflow stuff = devtools's job". So devtools is the storefront for the most common workflow functions from roxygen2, pkgload, etc.

It feels like the most self-consistent thing for usethis to do here would be to recommend running devtools::load_all(), the same way we recommend running devtools::document() in ~8 different places, versus calling it ourselves.

Obviously I don't care much about us adding one call to load_all(). It's more that the one-off nature of it feels odd.

malcolmbarrett and others added 2 commits February 26, 2021 08:30
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
@malcolmbarrett
Copy link
Collaborator Author

I think usethis has really trained me to expect that slight separation in workflows, as well. Whether or not that is intentional or the best approach, it's definitely lodged in my brain at this point!

@malcolmbarrett
Copy link
Collaborator Author

Oh, roxygen_update_ns() adds another load. Maybe I should wait until that issue is resolved before proceeding. I do like the roxygen_remind() rename, tho 🙃

@hadley
Copy link
Member

hadley commented Feb 26, 2021

@jennybc I was thinking that this would be a place for an experiment.

@jennybc
Copy link
Member

jennybc commented Feb 26, 2021

OK

@malcolmbarrett
Copy link
Collaborator Author

Ok, experiment commenced! One thing of note: I tried to make the communication about updating NAMESPACE and loading more usethis-like because it feels weird to have other styles of messages pop in (much like it did with gert originally). It also made the tests noisier.

Copy link
Member

@hadley hadley 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 — thanks for your work on this!

R/use_import_from.R Outdated Show resolved Hide resolved
R/use_import_from.R Outdated Show resolved Hide resolved
@jennybc
Copy link
Member

jennybc commented Feb 27, 2021

This should get a NEWS bullet.

@malcolmbarrett
Copy link
Collaborator Author

Ok, all updated! Thanks for iterating with me. Let me know if there's anything else!

@hadley
Copy link
Member

hadley commented Feb 27, 2021

I started playing around with this. I started by adding a check to see if the function actually existed, and then accidentally removed the line pasting funs together. But that made me wonder if one line per @importFrom is actually a better solution?

@hadley
Copy link
Member

hadley commented Feb 27, 2021

I've left a failing test to remind me to come back to this — block_append() probably needs to unique() what it writes out and should at least have an option to sort the output.

@malcolmbarrett
Copy link
Collaborator Author

Hm! I noticed that my original code did one line per element (unintentionally), and it wasn't too bad. I like that roxygen can put many imports on the same line, but in the wild it does get unwieldy quite quickly.

Now that you're saying that, it made me think that I ought to add a check that package is length 1 to avoid an unclear error message from glue.

@hadley
Copy link
Member

hadley commented Feb 27, 2021

I think one line per element is the right approach to avoid accidental duplication.

@malcolmbarrett
Copy link
Collaborator Author

Ok, I updated the last test to match that output

@hadley
Copy link
Member

hadley commented Mar 3, 2021

@jennybc I think it's worth you having another look — I've boosted the abilities of block_append() so that we create a more pleasant writing experience. I also removed the warnings from the template; I don't think they're that important since there's no real harm if you remove the comments, and it's certainly fine to edit the @importFrom statements.

@malcolmbarrett thanks for working on this!

R/use_import_from.R Outdated Show resolved Hide resolved
tests/testthat/test-block.R Outdated Show resolved Hide resolved
tests/testthat/test-block.R Outdated Show resolved Hide resolved
@malcolmbarrett
Copy link
Collaborator Author

malcolmbarrett commented Mar 3, 2021

Really like the addition of sort + unique, esp that it's the default for this use case. Edit: but should sort be an argument in use_import_from()? I would never use it (and in fact, I wish usethis were more forceful about use_tidy_description(), too), but I could see others wanting it

hadley and others added 2 commits March 3, 2021 10:53
Co-authored-by: Jennifer (Jenny) Bryan <jenny.f.bryan@gmail.com>
@hadley hadley merged commit 4819d19 into r-lib:master Mar 3, 2021
@hadley
Copy link
Member

hadley commented Mar 3, 2021

I don't think we need to make sort an argument — if you don't want it sorted, you can use some other approach 😄

@malcolmbarrett
Copy link
Collaborator Author

💯

Thanks for all the helpful feedback and contributions to this PR @hadley and @jennybc!

@jennybc
Copy link
Member

jennybc commented Mar 3, 2021

Should we now open an issue re: revisiting existing functions like use_pipe() or use_tibble() and route them through this code or deprecate them in favour of a call to this code, as appropriate?

@hadley
Copy link
Member

hadley commented Mar 3, 2021

Looks like we could use it in use_tibble() but not in use_pipe() or use_data_table() which work somewhat differently.

@malcolmbarrett
Copy link
Collaborator Author

I think it can supplement use_pipe() here:

usethis/R/pipe.R

Lines 32 to 35 in 4819d19

if (has_package_doc()) {
roxygen_ns_append("@importFrom magrittr %>%") && roxygen_remind()
return(invisible(TRUE))
}

I can file an issue and work on a corresponding PR

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.

Generalizing use_tibble() and use_data_table()
3 participants