Closes #163, #202 #197
Closes #163, #202 #197
Conversation
|
I'll update based on our conversation to change the default lint to |
…or-library-calls-to-be-at-top-of-script
nicholas-masel
requested review from
bms63,
kodesiba,
thomasp-b and
parmsam-pfizer
| # lint file if option is turned on | ||
| if (!is.logical(getOption('log.rx.lint'))) { | ||
| lint(file, getOption('log.rx.lint')) | ||
| lintr::lint(file, getOption('log.rx.lint')) |
There was a problem hiding this comment.
do we need :: if we have the function imported?
There was a problem hiding this comment.
I'm okay with leaving them in - we just haven't done a lot of :: in the past. I'm alway unclear what is the best practice.
There was a problem hiding this comment.
Here was the thought process for using ::. We no longer import the function to remove the dependency on lintr. So instead we warn users if they don't have lintr installed, using requireNamespaces() to check, and use lintr:: instead of attaching the package with require() to avoid unexpected changes to a users search path.
This was based on the second paragraph in the guidance here, https://cran.r-project.org/doc/manuals/R-exts.html#Suggested-packages.
There was a problem hiding this comment.
For packages that are not depends/imports, you need to use the package::function notation. @nicholas-masel is right that you wouldn't want to use require since it changes the environment (loading the package into the search path).
I also suggest naming the arguments just to make sure nothing gets mixed up.
| lintr::lint(file, getOption('log.rx.lint')) | |
| lintr::lint(filename = file, linters = getOption('log.rx.lint')) |
| #' linters = library_call_linter() | ||
| #' ) | ||
| #' | ||
| #' # okay |
There was a problem hiding this comment.
| #' # okay | |
| #' # Will pass lint checks |
| #' @examples | ||
| #' library(lintr) | ||
| #' | ||
| #' # will produce lints |
There was a problem hiding this comment.
| #' # will produce lints | |
| #' # Will fail lint checks |
There was a problem hiding this comment.
This looks pretty fantastic to me!!! @thebioengineer does this assuage the remotes issue?
| prefix = " ", initial = "")) | ||
| return() | ||
| } | ||
|
|
||
| # lint file if option is turned on | ||
| if (!is.logical(getOption('log.rx.lint'))) { |
There was a problem hiding this comment.
| if (!is.logical(getOption('log.rx.lint'))) { | |
| if (!is.logical(getOption('log.rx.lint'))) { |
Are you expecting the person to pass a viable lintr option? If so, maybe checking more explicitly rather than just if the option is not a boolean would be more reliable. something like is.list(getOption('log.rx.lint')) since it does get passed to lintr::lint as the linters?
| if (!requireNamespace("lintr", quietly = TRUE)) { | ||
| message(strwrap("Linting will not be included in the log. Install the | ||
| lintr package to use the log.rx.lint feature.", | ||
| prefix = " ", initial = "")) | ||
| return() | ||
| } | ||
|
|
There was a problem hiding this comment.
I would move this to within the if statement below. Not having lintr has no impact unless the choice is to try to lint if htat makes sense
| # lint file if option is turned on | ||
| if (!is.logical(getOption('log.rx.lint'))) { | ||
| lint(file, getOption('log.rx.lint')) | ||
| lintr::lint(file, getOption('log.rx.lint')) |
There was a problem hiding this comment.
For packages that are not depends/imports, you need to use the package::function notation. @nicholas-masel is right that you wouldn't want to use require since it changes the environment (loading the package into the search path).
I also suggest naming the arguments just to make sure nothing gets mixed up.
| lintr::lint(file, getOption('log.rx.lint')) | |
| lintr::lint(filename = file, linters = getOption('log.rx.lint')) |
bms63
deleted the
163-feature-request-consider-mandatory-lint-for-library-calls-to-be-at-top-of-script
branch
Thank you for your Pull Request!
We have developed a Pull Request template to aid you and our reviewers. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the logrx codebase remains robust and consistent.
The spirit of logrx
While many packages to facilitate the logging of code already exist in the R ecosystem, it is hard to find a solution that works well for clinical programming applications. Many logging implementations are more implicit and rely on user input to create the log for the execution of a script. While this is useful for logging specific events of an application, in clinical programming a log has a set purpose.
logrx is built around the concept of creating a log for the execution of an R script that provides an overview of what happened as well as the environment that it happened in. We set out to create a flexible logging utility that could provide the necessary information to anyone reviewing the code execution so they can recreate the execution environment and run the code for themselves. Please make sure your Pull Request meets this spirit of logrx.
Please check off each taskbox as an acknowledgment that you completed the task. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the
devbranch until you have checked off each task.log_remove().devtools::document()so all.Rdfiles in themanfolder and theNAMESPACEfile in the project root are updated appropriatelypkgdown::build_site()and check that all affected examples are displayed correctly and that all new functions occur on the "Reference" page.R CMD checklocally and address all errors and warnings -devtools::check()