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.

Sign up for GitHub

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

Jump to bottom

Some use_{internal,external}_XXX_file() functions fail without default value for argument name [BUG] #1060

Closed
ilyaZar opened this issue Jul 5, 2023 · 1 comment
Closed

Some use_{internal,external}_XXX_file() functions fail without default value for argument name [BUG] #1060

ilyaZar opened this issue Jul 5, 2023 · 1 comment
Assignees
@ColinFay
Labels
bug Something isn't working

Comments

@ilyaZar
Copy link
Contributor

ilyaZar commented Jul 5, 2023 •
edited
Loading

Describe the bug
The use_{internal,external}_XXX_file() family does not always provide default argument values for name but relies on:

if (missing(name)) {
  name <- basename(path)
}

This is fine when this happens at the top level of the function call. However, often there is check_name_length(name) before the if (missing(name))-part which produces an error.

To Reproduce
It's a bit cumbersome to provide a reprex, essentially one generates a new golem and invokes e.g. the use_external_file without a value for the argument name which runs into an error where the check_name_length(name) part gives:

...
argument "name" is missing, with no default
...

image

Additional context
@ilyaZar ilyaZar added the bug Something isn't working label Jul 5, 2023
ilyaZar added a commit to ilyaZar/golem that referenced this issue Jul 5, 2023
@ilyaZar
fix: set default value for argument name before using it
281de4e 
the order needs a change at some places: first set a default value when name argument is missing, only then call check_name_length() to avoid errors of the type:

"argument "name" is missing, with no default"

Refs: ThinkR-open#1060
ilyaZar added a commit to ilyaZar/golem that referenced this issue Jul 5, 2023
@ilyaZar
fix: set default value for argument name before using it
372d3aa 
- the order needs a change at some places: first set a default value when name argument is missing
    - only then call check_name_length() to avoid errors of the type:
    "argument "name" is missing, with no default"
- also, to avoid RStudio and other IDEs linting a missing argument, provide a default value "NULL" for argument  'name'

Refs: ThinkR-open#1060
@ilyaZar ilyaZar mentioned this issue Jul 5, 2023
Merged
5 tasks
VincentGuyader pushed a commit that referenced this issue Aug 8, 2023
@ilyaZar
Fix 1058, 1060, 1062 and improve code coverage in use_files.R from …
6d9bb1c 
…0 to 100% (#1059)

* tests: add dummy files for use_{external,internal}_XXX_files() testing

- add some dummy files to the inst/utils/ directory

- for later: add tests that download exactly from that directory

Refs: #1058

* tests: add testing for external-funcs standard cases

* tests: add testing for internal-funcs standard cases

- fix typo in testfile_template_html
- use styler::style_file(..., style = grkstyle::grk_style_transformer)

* fix: set default value for argument name before using it

- the order needs a change at some places: first set a default value when name argument is missing
    - only then call check_name_length() to avoid errors of the type:
    "argument "name" is missing, with no default"
- also, to avoid RStudio and other IDEs linting a missing argument, provide a default value "NULL" for argument  'name'

Refs: #1060

* feat: improve error handling and allow codecovr. to be 100%

- instead of throwing an error, return 'FALSE' is passed and cat_dir_necessary() prints an informative message to the user (with the same effect i.e. not creating a directory)
- this behavior is a bit more userfriendly and also allows for a code coverage of 100%; the latter is not possible to implement since tests are run non-interactively

* tests: add corner cases for tests to make covr. 100%
VincentGuyader added a commit that referenced this issue Aug 8, 2023
@VincentGuyader @ilyaZar
Dev 1059 (#1082)
7ba820b 
* Fix 1058, 1060, 1062 and improve code coverage in `use_files.R` from 0 to 100% (#1059)

* tests: add dummy files for use_{external,internal}_XXX_files() testing

- add some dummy files to the inst/utils/ directory

- for later: add tests that download exactly from that directory

Refs: #1058

* tests: add testing for external-funcs standard cases

* tests: add testing for internal-funcs standard cases

- fix typo in testfile_template_html
- use styler::style_file(..., style = grkstyle::grk_style_transformer)

* fix: set default value for argument name before using it

- the order needs a change at some places: first set a default value when name argument is missing
    - only then call check_name_length() to avoid errors of the type:
    "argument "name" is missing, with no default"
- also, to avoid RStudio and other IDEs linting a missing argument, provide a default value "NULL" for argument  'name'

Refs: #1060

* feat: improve error handling and allow codecovr. to be 100%

- instead of throwing an error, return 'FALSE' is passed and cat_dir_necessary() prints an informative message to the user (with the same effect i.e. not creating a directory)
- this behavior is a bit more userfriendly and also allows for a code coverage of 100%; the latter is not possible to implement since tests are run non-interactively

* tests: add corner cases for tests to make covr. 100%

* chore bump coverage

---------

Co-authored-by: Ilya Zarubin <[email protected]>
@ilyaZar
Copy link
Contributor Author

ilyaZar commented Aug 9, 2023

Closed via #1059 #1082

@ilyaZar ilyaZar closed this as completed Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ColinFay ColinFay

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ColinFay @ilyaZar