General Issue: Lintr CI check giving new and many findings

[bms63]

Background Information

Recently re-workings of CI workflows have generated more lintr findings. Unsure if these are false-positives or not.

Attached is file of the results from the latest lintr run
lintrs_findings.csv

Definition of Done

• Investigate why lintr is giving new findings (See Daphne's reply)
• If lintr is not giving false positives then fix all lintr issues

@pharmaverse/admiral @pharmaverse/admiral_comm Hi all - not totally sure what is happening with the lintr, but if anyone has some time to investigate what is going on and remedy??

[dgrassellyb]

@bms63 :
This is related to new CI workflows (since we are using latest versions of dependencies, we use now lintr:3.1.1 instead of lintr:3.0.2 so I guess this is related)

[bms63]

@bms63 : This is related to new CI workflows (since we are using latest versions of dependencies, we use now lintr:3.1.1 instead of lintr:3.0.2 so I guess this is related)

awesome!! then this is a great issue for folks wanting to get back into admiral or first time contributors!!

[esimms999-gsk]

@bms63 @dgrassellyb Question: When I start a brand new RStudio instance and clone admiral, the first thing I do is devtools::install_dev_deps(). This tells me that devtools is not installed. So, I install devtools and do devtools::install_dev_deps(). This works great. But it does not install lintr nor styler. Is it possible to install lintr and stylr to ensure the developer is using the same versions as the Action checks will use?

[bms63]

If you install the latest version of lintr and styler then you will have what is being used in the action

[esimms999-gsk]

True. But why leave that to chance? Why not have install_dev_deps do it?

[bms63]

Here is why we removed them from DESCRIPTION file. #1919

[dgrassellyb]

@bms63 / @esimms999-gsk : maybe we should then update the documentation as well on this (just talking about necessary dev dependencies to install like styler lintr) - I can add section on admiralci doc

[bms63]

@pharmaverse/admiral @pharmaverse/admiral_comm hey alll. Can someone look into fixing these lintr issues for us!! This will impact every future Pull Request as right now the CI action is failing. I actually think it might be a slightly bigger job than 1 hour...maybe 2-3 hours as there will be some investigation of each linter failing and maybe some quick decisions!?!

Anyways, it is a huge help to tackle this one!!

[gg106046]

@pharmaverse/admiral @pharmaverse/admiral_comm hey alll. Can someone look into fixing these lintr issues for us!! This will impact every future Pull Request as right now the CI action is failing. I actually think it might be a slightly bigger job than 1 hour...maybe 2-3 hours as there will be some investigation of each linter failing and maybe some quick decisions!?!

Anyways, it is a huge help to tackle this one!!

Hi @bms63, I'll give it a go!

[esimms999-gsk]

@gg106046 @bms63 I was digging into this a little bit. I think we need to implement a quick solution (stop lintr from failing so we can do PR's without problem) and then find a long-term solution (make all the lintr checks work as they should).

There are two types of lintr checks failing at this time:

• indentation_linter: "Indentation should be xx spaces but is yy spaces" and "Hanging indent should be xx spaces but is yy spaces".
• undesirable_function_linter: Function "structure" is undesirable. As an alternative, Use class<-, names<- and attr<- to set attributes"

For the indentation_linter, we have a problem with styler and lintr disagreeing: I did some testing on one of the files and if I made it pass styler, it failed lintr. And if it passes lintr, it failed styler. This will take some research to figure out the best way to handle this. But, for now, we can turn off this indentation_linter check by adding a line to the .lintr file right after the "object_usage_linter=NULL," line: indentation_linter=NULL. This will get rid of all the indentation_linter fails.

As for undesirable_function_linter, I tried similar but kept getting syntax errors wrt the .lintr file. I could find very little documentation on any of this and will post a question to Posit Community,

[esimms999-gsk]

https://community.rstudio.com/t/how-to-turn-off-undesirable-function-linter-in-lintr/181401

[esimms999-gsk]

Ack! I did not notice ... undesirable_function_linter is being ADDED to the checks; I think that by removing that line it is not used, by default.

Changing the .lintr file from:

linters: linters_with_defaults(
    line_length_linter(100),
    object_usage_linter=NULL,
    cyclocomp_linter(complexity_limit = 22),
    undesirable_function_linter = undesirable_function_linter()
  )
exclusions: list(
    "R/data.R" = Inf,
    "inst" = list(undesirable_function_linter = Inf),
    "vignettes" = list(undesirable_function_linter = Inf),
    "R/admiral_options.R" = list(line_length_linter = 8)
  )

... to this:

linters: linters_with_defaults(
    line_length_linter(100),
    object_usage_linter=NULL,
    cyclocomp_linter(complexity_limit = 22),
    #undesirable_function_linter = undesirable_function_linter()
    indentation_linter=NULL
  )
exclusions: list(
    "R/data.R" = Inf,
    "inst" = list(undesirable_function_linter = Inf),
    "vignettes" = list(undesirable_function_linter = Inf),
    "R/admiral_options.R" = list(line_length_linter = 8)
  )

... seems to work. I will create a branch and PR for this temporary solution. That will give us time to find a better, long-term solution.

[bms63]

@ddsjoberg @cicdguy @dgrassellyb im trying to come up to speed here and maybe misunderstanding...but how do other packages handle the divergence of styler and linter for code indentation?

[cicdguy]

@bms63 - usually that is accomplished by having specific linter configs per project, ones that conform with a style guide or style preference

[bms63]

@bms63 - usually that is accomplished by having specific linter configs per project, ones that conform with a style guide or style preference

did recent updates cause stlyer and linter to diverge? I'm guessing yes as we had been using older versions previously that matched.

I just would of thought they would match out of the box and we would customize to your project if you didn't like the defaults?

[cicdguy]

Indeed:
r-lib/lintr#2034
r-lib/lintr#2007

[esimms999-gsk]

PR #2322 has implemented the temporary solution: the failing checks are no longer being performed due to changes in the .lintr file (see above for before/after .lintr content). This allows us to continue with updates to admiral because the CI/CD lintr check no longer fails.
The long-term solution would be to figure out how to run, and pass, these checks. For indentation_linter checks, we need to decide what the indentation rules should be and ensure our settings for stylr and lintr are in agreement. For the undesirable_function_linter check (Function "structure" is undesirable. As an alternative, Use class<-, names<- and attr<- to set attributes), we need to learn why the base function "Structure" is not desirable and make the appropriate changes to any code which uses this.

While working on this, all of the lintr errors can be surfaced by making use of the original .lintr file:

linters: linters_with_defaults(
    line_length_linter(100),
    object_usage_linter=NULL,
    cyclocomp_linter(complexity_limit = 22),
    undesirable_function_linter = undesirable_function_linter()
  )
exclusions: list(
    "R/data.R" = Inf,
    "inst" = list(undesirable_function_linter = Inf),
    "vignettes" = list(undesirable_function_linter = Inf),
    "R/admiral_options.R" = list(line_length_linter = 8)
  )

[esimms999]

@bundfussr I think you wrote this line of code in call_derivation.R a couple of years ago, so I will ask you!
The latest version of lintr is complaining about the function "structure". In order to suppress the errors, I commented the undesirable_function_linter = undesirable_function_linter() line in the .lintr file until we can figure out the suggested fix. Do you understand how to re-write this line so that lintr will be happy? I did try replacing it with class(args) <- c("params", "source", "list") but the tests fall down.

[bundfussr]

I did try replacing it with class(args) <- c("params", "source", "list") but the tests fall down.

@esimms999 , you need to replace it with

  class(args) <- c("params", "source", "list")
  args

I think structure() returns the object while class() doesn't do this. Therefore args needs to be added to achieve that params() returns the object.

[esimms999-gsk]

@bundfussr Thanks, I'll give that a try!

Opened issue #2344 to concentrate on undesirable_function_lintr errors.

[esimms999-gsk]

OK, some success. The change @bundfussr suggested, above, worked.

And I successfully made other changes, but I am sure there is a better way than changing 4 lines to 8, e.g. in test-user_utils.R:

 input <- tibble::tibble(
    a = structure(c("a", "b", "", "c"), label = "A"),
    b = structure(c(1, NA, 21, 9), label = "B"),
    c = structure(c(TRUE, FALSE, TRUE, TRUE), label = "C"),
    d = structure(c("", "", "s", "q"), label = "D")
  )

to:

 input <- tibble::tibble(
    a = c("a", "b", "", "c"),
    b = c(1, NA, 21, 9),
    c = c(TRUE, FALSE, TRUE, TRUE),
    d = c("", "", "s", "q")
  )
  attr(input$a, "label") <- "A"
  attr(input$b, "label") <- "B"
  attr(input$c, "label") <- "C"
  attr(input$d, "label") <- "D"

There are still some changes which need to be made and I am unsure how to do them; I am not having much luck finding information on this with Google. An example:

admiral_environment$nmap <- structure(
        temp_not_mapped,
        class = union("nmap", class(temp_not_mapped)),
        by_vars = vars2chr(by_vars_left)
      )

and:

structure(class = c("adam_templates", "character"), package = package)

[bms63]

Any ideas @pharmaverse/admiral @pharmaverse/admiral_comm

[bundfussr]

You could replace

a = structure(c("a", "b", "", "c"), label = "A"),

with

a = `attr<-`(c("a", "b", "", "c"), "label", "A"),

If you want to set more than one attribute you can use attributes(). For example

attributes(temp_not_mapped) <- list(
  class = union("nmap", class(temp_not_mapped)),
  by_vars = vars2chr(by_vars_left)
)

or if you want to assign the object to a variable

admiral_environment$nmap <- `attributes<-`(
  temp_not_mapped,
  list(
    class = union("nmap", class(temp_not_mapped)),
    by_vars = vars2chr(by_vars_left)
  )
)

[esimms999-gsk]

@bundfussr I had success with

a = `attr<-`(c("a", "b", "", "c"), "label", "A")

but the tests fail when I replace

      admiral_environment$nmap <- structure(
        temp_not_mapped,
        class = union("nmap", class(temp_not_mapped)),
        by_vars = vars2chr(by_vars_left)
      )

in derive_merged.R with:

admiral_environment$nmap <- `attributes<-`(
  temp_not_mapped,
  list(
    class = union("nmap", class(temp_not_mapped)),
    by_vars = vars2chr(by_vars_left)
  )
)

Unfortunately, I am really out of my depth on this.

I have done what I can at this time but will need to re-visit this when I have more understanding or find someone to assign it to.

[ddsjoberg]

It's important to keep the code readable, which makes it easier to maintain.

My suggestion is to either keep the structure() call and fence it in chunk where lintr is turned off, or to just assign labels one at a time, even though the overall code is longer.

[esimms999-gsk]

I am just assuming that lintr's complaining about the structure() function is based upon some wisdom I am unaware of.

Asking the reason why: Posit Community

[esimms999-gsk]

Posit Community response points at: r-lib/lintr#2227, which has this great comment:

@ddsjoberg I agree 100% that keeping the code readable and easier to maintain is high priority. I will fence in structure() calls so that lintr ignores them. This will allow the undesirable_function_linter to be turned on to catch anything else.

[esimms999-gsk]

The undesirable_function_linter problem has been resolved by making use of alternate syntax in building test-related tibbles and by using # nolint on other code.
As for the indentation_linter problem, this was discussed in the :{admiral} core dev team stand-up" meeting on 06Mar2024: @bundfussr pointed out that there are automated styler checks which will catch anything indentation-related and there is no need to repeat the check with lintr. Therefore, we can keep the indentation_linter checks switched off in .lintr, and depend on the styler check.
Closing the issue.