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

Restrict the number of file names provided in use_r() and use_test() #862

Merged
merged 9 commits into from Oct 16, 2019
Merged

Restrict the number of file names provided in use_r() and use_test() #862

merged 9 commits into from Oct 16, 2019

Conversation

@strboul
Copy link
Contributor

@strboul strboul commented Aug 2, 2019

Although it will be quite useful that if we have a way to create multiple files at once - I might use them as stubs and fill them in once I feel ready, it is not so easy to do the implementation at the moment.

So I thought that we can limit these calls that they will stop the execution if a vector provided with which its length is more than one. That limitation is also aligned with the current API of these functions because the main argument either in use_r() or use_test() states 'name' (not the plural version 'names').

This is my first PR in usethis. I am happy to modify my changes accordingly based on your review.

Resolves #861

R/r.R Outdated
@@ -8,6 +8,7 @@
#' Packages](https://r-pkgs.org).
#' @export
use_r <- function(name = NULL) {
check_file_name_length(name)
Copy link
Member

@jennybc jennybc Aug 5, 2019

Choose a reason for hiding this comment

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

I think it would be pretty self-consistent of us to do something simpler in this case. I propose stopifnot(is_string(name)) instead of a custom helper.

Copy link
Member

@jennybc jennybc Aug 5, 2019

Choose a reason for hiding this comment

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

Also, the check should happen after the name <- name %||% ... line, because NULL input is valid.

Copy link
Contributor Author

@strboul strboul Aug 6, 2019

Choose a reason for hiding this comment

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

I agree all except that we use stopifnot(). A regular if-not-true-stop branch might be more useful to give better error messages for the end-users.

Copy link
Contributor Author

@strboul strboul Aug 6, 2019

Choose a reason for hiding this comment

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

On the other hand, I see that stopifnot(is_string(name)) is often used in the package to check inputs.

tests/testthat/test-use-r.R Outdated Show resolved Hide resolved
R/test.R Outdated Show resolved Hide resolved
strboul and others added 5 commits Aug 6, 2019
As proposed in review, a new helper for checking
length of input strings isn't pretty much needed.

The `is_string()` call in the utils is convenient.
Therefore, the check is moved inside the
`check_file_name()` helper which is used both
by `use_r()` and `use_test() calls.
@hadley hadley merged commit 3996db9 into r-lib:master Oct 16, 2019
0 of 2 checks passed
@hadley
Copy link
Member

@hadley hadley commented Oct 16, 2019

Thanks @strboul!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants