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

Extend 'on_cran' for #14 #15

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Extend 'on_cran' for #14 #15

wants to merge 6 commits into from

Conversation

mpadge
Copy link

@mpadge mpadge commented May 14, 2024

@briandconnelly This is a draft PR to extend your on_cran function to work properly in all environments beyond devtools/testthat. The defaults are taken from the fda package (no public source, so CRAN tarball only), and the only real difference between this and their version is that theirs allows the CRAN_pattern to be a vector of arbitrary length, although it defaults to the single value used here. The default n_CRAN_envvars = 5L is also theirs.

@briandconnelly
Copy link
Owner

Looks good to me! Thank you for all the background information!

The logic makes sense to me. I can't say I have any strong preferences between defaulting to a threshold of 5 or 10, but I think consistency with {fda} is a solid idea.

Aside from mocking up some environments and testing our own assumptions, I'm not sure how we'd test its accuracy. I'm ok with that, though. Your research seems to be exhaustive.

I'd say we move forward with it! My only asks are a few tests to make sure it behaves as expected and updates to the documentation for on_cran(). If you're up for it, I'd say you should add yourself as a contributor in the DESCRIPTION file.

@mpadge
Copy link
Author

mpadge commented May 23, 2024

Thanks Brian, that should now be all done. Note that the need for this came out of a (private) discussion in rOpenSci slack, where this new functionality will be announced. We'll also follow up with a public shout-out in our monthly newsletter. I'll ping you from PR for that as soon as it's there. Thanks for agreeing to provide such a great home for this function! (And of course happy to add any further minor tweaks if you like - just let me know.)

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.

None yet

2 participants