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

Provide use_compat() for managing common compat-[pkg].R files #1654

Closed
wurli opened this issue Jul 25, 2022 · 19 comments · Fixed by #1748
Closed

Provide use_compat() for managing common compat-[pkg].R files #1654

wurli opened this issue Jul 25, 2022 · 19 comments · Fixed by #1748
Labels
feature a feature request or enhancement tidy ✨ tools for the tidyverse team

Comments

@wurli
Copy link

wurli commented Jul 25, 2022

Often when writing a new package I find myself simply lifting scripts like compat-purrr.R from the {rlang} source-code to avoid taking on more dependencies than necessary. I think formalising this process would be well worth the effort. E.g. it could be done as follows:

  • use_compat(pkg) would copy a compat-pkg.R into the user's R directory, e.g. from the github for an existing package such as {rlang} where the 'master' version is defined, or from a dedicated repo for tidyverse compat- files. The second option seems better to me as it would also provide a nice place to look for such files without necessarily having to go through {usethis}.
  • Re-running use_compat() after the 'master' version has changed could update the existing compat- script after prompting the user to confirm any (potentially breaking) changes.
  • use_compat() could also support single functions, e.g. in the case of withr::defer(), which has its own compat-defer.R file in the {rlang} source code.
  • Specifically, I think standardising 'compat' files for at least the following packages would be really useful: {cli}, {dplyr}, {forcats}, {glue}, {purrr}, {rlang}, {stringr}, {vctrs}, {withr} and {zeallot}

Obviously the real value here would be in having such compat- scripts readily available. However it sees like this is already somewhat of a 'soft-supported' service from the Tidyverse, to wit, rlang/compat-vctrs.R begins with the following line:

# Latest version: https://github.com/r-lib/rlang/blob/main/R/compat-vctrs.R

So, perhaps the work is already largely done and the creation of use_compat() would mainly be a case of compiling all compat- files which already exist.

@jennybc
Copy link
Member

jennybc commented Aug 29, 2022

This suggestion feels along the same lines as use_github_action(), where we help folks pull copies of workflow config files from r-lib/actions.

Do you have any thoughts on this @lionel-?

@jennybc jennybc added feature a feature request or enhancement tidy ✨ tools for the tidyverse team labels Aug 29, 2022
@DavisVaughan
Copy link
Member

See also r-lib/rlang#1401

I do think it would be useful. We didn't add it to rlang immediately because we thought we might need some kind of dependency system (i.e. compat-foo.R might need compat-bar.R to work, which does occur in some rlang compat files). But maybe we can do without that for now, and just document the dependencies like this https://github.com/r-lib/rlang/blob/b45acd370617c307e72df8a60e7ef35ad1b0c05a/R/compat-types-check.R#L6

@jennybc
Copy link
Member

jennybc commented Aug 29, 2022

Ah, I didn't realize this was already under way in rlang.

@lionel-
Copy link
Member

lionel- commented Aug 30, 2022

yup I've been meaning to take a look at implementing a simple dependency and version system for the rlang compat files. Also it would be nice to record all the compat files used by the package in a file so that you could do usethis::update_compat().

A few years ago @hadley thought this wouldn't be worth the work, but since we have now more compat files than we used to, I think this might now be worth it. I especially like that compat files allow us to refine experimental APIs without causing revdep breakages. They are a good playing field for emerging ideas and some more automatic tooling would be helpful.

@wurli
Copy link
Author

wurli commented Aug 30, 2022

Thanks for the comments, I also wasn't aware of the discussion underway in rlang. Regarding dependencies, I really like the idea of documenting dependencies on other compat- files, but this might get tricky. E.g. consider the following:

  • My package {foo} imports {rlang}
  • {foo} also uses a compat-purrr.R script
  • compat-purrr.R has a dependency on compat-rlang.R, but the {rlang} import makes this redundant whilst also potentially changing behaviour in some edge cases.

Options of dealing with this are:

  1. Develop a more sophisticated system for managing dependencies in compat- files (perhaps the simplest implementation is to use {pkg} if available and compat-pkg.R if not?)
  2. Make all compat- files dependency-free

To be honest I would lean towards option 2, since building up a sophisticated dependency system for compat- files seems to defeat the point of having them in the first place.

@lionel-
Copy link
Member

lionel- commented Aug 30, 2022

compat-purrr.R has a dependency on compat-rlang.R, but the {rlang} import makes this redundant whilst also potentially changing behaviour in some edge cases.

Just to be clear compat-purrr.R uses rlang directly, it doesn't depend on compat-rlang.R. But other files use compat-rlang.R so that they only use rlang conditionally, if installed. Currently the functionality in compat-rlang is inlined in those files.

We also have compat-types-check.R which depends on compat-obj-type.R. In this case the functionality is not inlined and you have to manage the dependency yourself.

I would like something (a) more consistent and (b) with less manual work. What I don't want is a "sophisticated" dependency system, we should only consider a simple one.

@wurli
Copy link
Author

wurli commented Aug 30, 2022

Just to be clear compat-purrr.R uses rlang directly, it doesn't depend on compat-rlang.R. But other files use compat-rlang.R so that they only use rlang conditionally, if installed. Currently the functionality in compat-rlang is inlined in those files.

Ah - thanks for clarifying 👍

I would like something (a) more consistent and (b) with less manual work. What I don't want is a "sophisticated" dependency system, we should only consider a simple one.

Do you think these requirements would be met by limiting the scope of use_compat()? E.g. by assuming each each compat- file will cover single package as opposed to a feature/function?

Apologies if I'm a step behind here - I'm not sure if compat-types-check.R is providing compatibility with another package or not.

@DavisVaughan
Copy link
Member

it would be nice to record all the compat files used by the package in a file

Random thought that we might could do:

Config/usethis/compat:
    r-lib/rlang/purrr,
    r-lib/rlang/types-check

Which usethis::use_compat() would automatically parse and pull in

@lionel-
Copy link
Member

lionel- commented Aug 30, 2022

Interesting idea.

@malcolmbarrett
Copy link
Collaborator

So we're all aware while thinking about this, two pre-existing packages deal with a similar problem:

I haven't seen much about them since they first came out, though, so I don't think they much took off. One of the authors here seems to be frustrated that tools he's developed are ignored without discussion, so I thought it best to bring it up.

@jennybc
Copy link
Member

jennybc commented Aug 30, 2022

I think the packages above are solving a different problem. Maybe I'd call them "compat packages"?

I think the problem we are trying to solve is how we vendor .R files from one package to another. This has some of the same motivations as "compat packages" (lack of dependencies), but I don't see the overlap between what we're discussing here and the actual packages above.

@malcolmbarrett
Copy link
Collaborator

malcolmbarrett commented Aug 30, 2022

freebase is like that, actually. It stores files that get copied into a package rather than being a dependency itself. In other words, it deals with single compat files, as well, and in fact, uses usethis-style code to do so. bplyr is what you call a compat package, though.

@lionel-
Copy link
Member

lionel- commented Aug 30, 2022

Vendoring an entire package is something that @gaborcsardi does often and has tools for, IIRC. But I agree with Jenny that this is a different problem than single compat files.

@gadenbuie
Copy link
Contributor

Another solution in this space is staticimports to statically import functions from another package. It uses a roxygen2-style import syntax and imports individual functions with automatic function-dependency resolution. It requires exported functions be placed in inst/staticexports though, but I think the approach could be extended to package code.

What's nice about staticiports is that, in theory, you could import check_bool() from rlang without worrying about where it comes from or its dependencies — staticimports will automatically inline stop_input_type() and check_bool()'s other function deps without you having to copy both compat-types-check.R and compat-obj-type.R.

@hadley
Copy link
Member

hadley commented Jan 18, 2023

If we're going to make this work, I think we're going to need the compat files to have more machine readable structure. I'd suggest we mimic the top of a .Rmd, like so:

# ---
# src-repo: r-lib/rlang 
# src-file: compat-types-check.R
# dependencies: [compat-obj-type.R]
# last-updated: 2022-08-11
# ---
#
# ## Changelog
# 
# 2022-08-11:
# - Added changelog.
# 
# nocov start 

...

# nocov end

And then I think we need some additional text at the top of the copied files to make it clear that they come from another repo, maybe just something like:

# ---------------------------------------------------------------------------------
# Compatiblity file inlined from rlang/R/compat-types-check.R: do not edit by hand
# ---------------------------------------------------------------------------------

Maybe we should also give the created compat file a different prefix?

@hadley
Copy link
Member

hadley commented Jan 18, 2023

Actually maybe the src file doesn't need src-repo and src-file since they can be automatically added.

@hadley
Copy link
Member

hadley commented Jan 18, 2023

Or maybe just a src like https://github.com/r-lib/rlang/blob/main/R/compat-cli.R.

@hadley hadley changed the title [feature request] Implement use_compat() for managing common compat-[pkg].R files Provide use_compat() for managing common compat-[pkg].R files Jan 18, 2023
@jennybc
Copy link
Member

jennybc commented Jan 18, 2023

Just re-read this for first time in a while. If you work on it @hadley, use_github_file() is a very handy function for this sort of thing.

@hadley hadley added this to the v2.2.0 milestone Jan 19, 2023
hadley added a commit that referenced this issue Jan 22, 2023
@hadley
Copy link
Member

hadley commented Jan 30, 2023

Naming suggestion from @lionel-:

standalone-purrr.R        # Source
import-standalone-purrr.R # Target

hadley added a commit that referenced this issue Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement tidy ✨ tools for the tidyverse team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants