[WIP] Bugfix/missing lintr action #139
Conversation
| # Need help debugging build failures? Start at https://github.com/r-lib/actions#where-to-find-help | ||
| on: | ||
| push: | ||
| branches: [main, master] |
There was a problem hiding this comment.
Do we also want to lint when someone pushes a feature branch (so devs get earlier feedback if something should be fixed)?
There was a problem hiding this comment.
This is the standard lintr workflow maintained by r-lib and I feel it should be good enough for our purposes. I imaging folks will get in the habit of linting before pushing just as we normally test and chack locally before pushing too.
| @@ -19,7 +34,7 @@ check_input <- function(input, property, parent_schema, | |||
| return() | |||
| } | |||
| } | |||
| if (!is.atomic(input) | is.pairlist(input)) { | |||
| if (!is.atomic(input) || is.pairlist(input)) { | |||
There was a problem hiding this comment.
In case there are other newish R folks following along, I found an explanation of | vs || here: https://stat.ethz.ch/R-manual/R-devel/library/base/html/Logic.html
| @@ -24,9 +26,9 @@ load_model_metadata <- function(hub_path, model_ids = NULL) { | |||
| UseMethod("load_model_metadata") | |||
| } | |||
|
|
|||
|
|
|||
| # cyclomatic complexity of 17 instead of 15 acceptable | |||
There was a problem hiding this comment.
Is this something worth putting in the common .lintr file, or is the increased complexity of 17 specific to this file? (e.g., cyclocomp_linter(complexity_limit = 17L))
There was a problem hiding this comment.
I don't think 15L is particularly meaningful or that 17L would be more appropriate. I think the particular linter is useful to bring attention to extra cyclomatic complexity but also ok for the developer to ignore if they feel it's warranted
| @@ -36,7 +36,7 @@ | |||
| model_id_merge <- function(tbl, sep = "-") { | |||
| # check all required columns present | |||
| if (!all(c("model_abbr", "team_abbr") %in% names(tbl))) { | |||
| missing_cols <- c("model_abbr", "team_abbr")[ | |||
| missing_cols <- c("model_abbr", "team_abbr")[ # nolint: object_usage_linter | |||
There was a problem hiding this comment.
Another question about the hubverse's common .lintr... I see object_usage_linter in several of this PR's nolint directives. Is this something we want to ignore globally, or is it specific to hubUtils?
If the latter is true, would it be worth supplying a project-specific set of configs to this repo (mostly as a convenience in case people forget to add the exception when coding)?
There was a problem hiding this comment.
There's discussion regarding this here: https://github.com/orgs/Infectious-Disease-Modeling-Hubs/discussions/3#discussioncomment-8477805 & #141
In short, it's useful to not turn off the linter completely but it is problematic with cli messages while purrr calls can avoid triggering false linter positives by switching out formula expressions
|
Thanks @bsweger ! Added responses in comments |
For some reason, on the packages I upgraded last friday, the linting workflow was missed off (
probably used an earlier installed version ohI use put the wronghubDevshubDevsconfiguration function in my upgrade issue template 🤦♀️)Adding the missing workflow here as well as updating R CMD CHECK & codecov worklfows with newer versions.
Also fixing linting issues and cleaning up in preparation for package split.
Also reverts some
nolintcomments by usingfunction(x)calls inpurrrinstead of~(as dicsussed in #141 and https://github.com/orgs/Infectious-Disease-Modeling-Hubs/discussions/3#discussioncomment-8477805)