feat: introduce lintr

[gegnew]

• Add .lintr config
• Format and lint all files
• clamp_q -> clampQ
• Reduce cyclomatic complexity of getEvents
• Reduce cyclomatic complexity of getStatistics
• Add linting workflow

This was formatted with the styler/tidyverse defaults (https://styler.r-lib.org/index.html).

Q's:

• Do we like this formatting style?
• Should I add a pre-commit hook for styler? Currently the use of styler is not enforced anywhere.

[zbjornson]

Happy to see this.

Do we like this formatting style?

Yes, though I wish it allowed = instead of <-. Surprisingly no one has asked for that in lintr's issues.

Should I add a pre-commit hook for styler? Currently the use of styler is not enforced anywhere.

Maybe wait on this and see how it goes? I don't love pre-commit hooks because they slow things down. It would be nice if styler could run in a "report-only" mode.

[zbjornson]

Also looks like it can be left enabled.

[gegnew]

If it's left enabled, I get the following errors:

R/util.R:91:11: warning: no visible binding for global variable ‘lastError’
  pkg.env$lastError
          ^~~~~~~~~
R/util.R:146:16: warning: no visible binding for global variable ‘_id’
  val <- vals$`_id`
               ^~~

The first I can fix by adding pkg.env$lastError <- NULL at the top of util.R. The second I could just skip with # nolint.

But one of the biggest reasons to use lintr is for IDE integration, imho, and unfortunately any package-internal functions (i.e. those that aren't included in the roxygen-generated NAMESPACE) will threw an error (see r-lib/lintr#482).

Additionally, object_usage_linter executes package code with eval during checks, which is potentially a bit dangerous (although there aren't any other contributors to the package, so it's probably not a huge concern).

[zbjornson]

Okay, fine to leave it then.

(I'd like to get rid of getLastError btw. R has better facilities with try/catch I think.)

Just for my understanding, is the second one a false-positive? We have other code like getStatistics.R:161 percentOf <- serverPops$`_id`

[gegnew]

#73

[gegnew]

I was wondering as well if it was a false positive. At any rate, I managed to quiet the error

[zbjornson]

It does indeed appear to be a bug in lintr. I haven't tried to make a minimal repro to open an issue there. How'd you quiet the error?

[zbjornson]

Also: need to build the .Rd files!

[zbjornson]

Looks good, one tiny comment below and another above. ⬆️

[zbjornson]

Here we go, it's CRAN's policy. https://cran.r-project.org/doc/manuals/r-release/R-exts.html

The mandatory ‘Title’ field should give a short description of the package. Some package listings may truncate the title to 65 characters. It should use title case (that is, use capitals for the principal words: tools::toTitleCase can help you with this), not use any markup, not have any continuation lines, and not end in a period (unless part of …). Do not repeat the package name: it is often used prefixed by the name. Refer to other packages and external software in single quotes, and to book titles (and similar) in double quotes.

So maybe it will be happy with 'CellEngine' API toolkit? Not at my computer to try it.

[zbjornson]

maybe it will be happy with 'CellEngine' API toolkit?

Confirmed, single quotes make it happy. I don't care if it's API toolkit for 'CellEngine' or 'CellEngine' API toolkit, but please add the single quotes per the guide above, then I'm good!