using lintr

[nickreich]

Would it make sense to implement (sooner rather than later) lintr for the hubverse packages? I have it set up on another package (with thanks to Hugo Gruson who contributed it) and it seems to enforce more consistent stylistic practices. Seems like it'd be good when we have lots of people contributing code.

[sbfnk]

Yes I agree it’s a good thing. Just makes code more readable overall.

[bsweger]

I agree that linters are great when there are a number of contributors. When you're onboarding, it helps to know that your code style is one less thing to worry about because the computer will flag anything that doesn't align.

[elray1]

I use lintr and while I find it extremely helpful overall, about once a month I choose to ignore its suggestions -- e.g., I often find it helpful to type "things that look like code" in comments, and lintr will complain about that (here is an example where I ran into this recently). Overall, I find it to be extremely helpful, though. I guess my suggestion is that we use lintr but allow for occasional thoughtful overrides of its complaints.

[bsweger]

I think you can add an option to your .lintr file to exclude comments and make other tweaks (e.g., use a line length other than 80 characters).

[bsweger]

If we end up using lintr, a "hubverse approved" .lintr config would be great to include in @annakrystalli's proposed template.

[LucieContamin]

I agree with the using it but also allow for some overrides when necessary

[annakrystalli]

Regarding linting, what I've done so far through hubDevs is add a linting GitHub Action as part of the standard hubverse package workflows: https://github.com/Infectious-Disease-Modeling-Hubs/hubDevs/blob/main/R/use_hubdev_pkg_actions.R

What I haven't implemented though is @bsweger's suggestion of adding a standard hubverse .lintr file through hubDevs.

This would probably be a good next step to give ourselves some leeway as it's quite irritating for linters to throw errors for e.g. long urls. Here's a link to a draft _lintr template file I've added to hubDevs which could be a good starting point. Other suggestions to be included welcome. Once we've somewhat settled, I'll add the functionality to actually include the template as part of package set up.

[nickreich]

This all looks good to me! Seems like it will be a somewhat cumbersome task to go through each of the existing packages and add lintr, with the corrections needed, right? Here is an example of a PR for another package that I wrote being fixed up using lintr. Maybe it's relatively simple to do this, but might take a concerted effort from a few people off and on for a few days to get this done.

[annakrystalli]

The idea is to update all packages as I go through them to update to the hubverse so shouldn't be much effort. Adding a .lintr file will be part of use_hubdev_pkg_actions().

[annakrystalli]

Another tool I quite like to use too as it takes care of of many of the indentation/long lines linting issues it the styler package. I use the RStudio addins all the time.

[bsweger]

For visibility, moving some questions here that I asked in Slack (sorry 😅 !)

Very excited for our glorious future of linting! A few questions about roll-out and "enforcement":

1. Is the intent to add CI checks that fail if code pushed to a branch doesn't lint (I think those checks would only run on updated code, not on the repo's existing code)?
2. Would we implement linting organically, as older files change, or would we do an initial "lint and do a one-time initial fix on everything" PR for our repos?

[bsweger]

And posting @elray1 's input from Slack:

1. yes? enforcing style through non-human, automated means sounds good to me
2. i vote for organically. i’m basically ready to move on from many of our older packages, i’m not sure it’s worth sinking time into them. [note: the Slack was also discussion Reich Lab-specific packages which I left out in this wider Hubverse convo]

[elray1]

refining my answer to part 2: i don't want to do this for all reich lab packages, but I do think it'd be good to standardize styling across all hubverse packages, with an intentional push at some point if needed to get there?

[annakrystalli]

After being half way through the linting process with hubUtils, just wanted to revisit this dicsussion as I'm not 100% sure this is providing more value than it's worth for us currently.

A large part of fixes seem to be false positives of the object_usage_linter which does not recognise use of objects in cli messages or in ~ expressions in purrr. I have not found a way to fix this through configuring the .lintr file, This motivated me to see what the tidyverse devs do in their packages as they use purrr and cli heavily and noticed that in their popular packages they do not use lintr GitHub Action workflows. instead they use an action that automatically builds documentation and styles the package using styler.

I'm not sure what to suggest here but the enforcement of lintr atm is requiring our code to be peppered all over the place with nolint comments which to me feels like polluting the code but more importantly requires unnecessary time and effort.

There have been some useful issues picked up but unfortunately these have been few and far between.

Would welcome thoughts

[sbfnk]

Thanks for sharing, really interesting. If you find lots of false positives with one linter would it not be an option to completely deactivate this particular one in .lintr? I'd say that's preferable to nolint statements which can make long-term maintenance harder.

Can you share a few examples of lines that are being flagged / you've been marking with nolint? We've been curating a .lintr file to avoid a few idiosynchrasies but haven't come across this one yet I think:
https://github.com/epiverse-trace/packagetemplate/blob/main/.lintr

[annakrystalli]

Here's an example where an object is being created: https://github.com/Infectious-Disease-Modeling-Hubs/hubUtils/blob/4755581fe56c680316736c3cc6f55fac582967db/R/create_hub_schema.R#L211 and then just a few lines below it is being used on the rhs of a ~ expression within a purrr::map() call: https://github.com/Infectious-Disease-Modeling-Hubs/hubUtils/blob/4755581fe56c680316736c3cc6f55fac582967db/R/create_hub_schema.R#L215

It appears that lintr consistently fails to detect use of variables in the lhs of ~ expressions as well as in messages generated with cli (although it does have a setting to check glue calls (which I'm guessing used to give them similar issues)

I could certainly completely deactivate object_usage_linter overall but it did pick up a couple of stale variables so I'm hesitant to.

Here's the full PR containing all the changes made to conform to lint: https://github.com/Infectious-Disease-Modeling-Hubs/hubUtils/pull/139/files.

There a few good nuggets of useful changes (and just looking at the code helped uncover a bug as well as update a superceded function) but it took some time to go through it all (admittedly hubUtils is the largest hubverse pkg). That's not so much my issue as much as I feel a majority of the changes are inconsequential or they actually reduce the quality of the code because of the sprinkling on nolint comments throughout.

[sbfnk]

I see, thanks for the detailed followup! I tried replacing the call with

purrr::map_chr(
    str_partitions,
    \(x) names(str_arrow_datatypes)[x == str_arrow_datatypes]
  )

and this seemed to make the linter happy. Perhaps rewriting them in this way might be a better option than adding nolint tags? Depends on how far backwards we want to be compatible with R versions I guess, and obviously annoying one way or the other, and no help with the cli issues.

[annakrystalli]

Oh nice! Definitely worth that edit instead of adding nolint comments.

Still need to find a solution for the cli messages but might look to see how the linter deals with glue strings and see if I can replicate the same approach for cli strings.

[annakrystalli]

Hehe... Of course @Bisaloo has already opened an issue in lintr about the feature we want! 😄 r-lib/lintr#2252