Skip to content

use_data(): match R dependency to version #1672

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

Merged
merged 5 commits into from
Jan 19, 2023

Conversation

dpprdan
Copy link
Contributor

@dpprdan dpprdan commented Sep 12, 2022

version = 3 is only supported by R >= 3.5.0, so this should be the minimal dependency version, IMHO.

Are there tests for use_dependency() in use_data()? I haven't found any (but possibly not looked hard enough?).

Should version default to NULL or 3 now or anytime soon? 2 feels outdated for current packages. I guess there is something to be said for a fixed version as default (instead of NULL = default depending on (local) R version), but the downside is that it gets outdated. It's probably better to nudge users into setting the desired version themselves (via use_data_raw()) instead of relying on the default?

@hadley
Copy link
Member

hadley commented Jan 17, 2023

Agreed that we should do better here, but I don't think setting to 3.5 like this is the right approach. I think it would be to just check the current version and tell the user if they need to increment it. Do you want to try implementing that?

@dpprdan
Copy link
Contributor Author

dpprdan commented Jan 19, 2023

I think it would be to just check the current version and tell the user if they need to increment it.

What's your motivation? The "when you use version 3 you get an R v3.5 dependency" gotcha? Or something else?

You're probably aware, but use_dependency() only upgrades if min_version > existing_version and issues a message if it sets or upgrades R in Depends.

> use_data(mtcars, version = 2)
✔ Adding 'R' to Depends field in DESCRIPTIONCreating 'data/'Setting LazyData to 'true' in 'DESCRIPTION'Saving 'mtcars' to 'data/mtcars.rda'Document your data (see 'https://r-pkgs.org/data.html')
> use_data(mtcars, overwrite = TRUE, version = 3)
✔ Increasing 'R' version to '>= 3.5.0' in DESCRIPTIONSaving 'mtcars' to 'data/mtcars.rda'Document your data (see 'https://r-pkgs.org/data.html')

I don't fully grasp yet in what scenario setting 3.5 (vs. 2.10 I suppose?) like this is problematic, I suppose.

@hadley
Copy link
Member

hadley commented Jan 19, 2023

Oh I didn't know that. This approach should be fine then.

@hadley
Copy link
Member

hadley commented Jan 19, 2023

So to finish off this PR, can you please add a bullet to the top of NEWS.md? It should briefly describe the change and end with (@yourname, #issuenumber).

@dpprdan
Copy link
Contributor Author

dpprdan commented Jan 19, 2023

Done, thanks!

@hadley hadley merged commit f05f710 into r-lib:main Jan 19, 2023
@dpprdan dpprdan deleted the fix/use_data_r_version branch January 19, 2023 17:00
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