Streamline and tighten lintr checks

[Bisaloo]

Fix #218

• 325212a replaces the workflow to only run in changed files
• c7ecfb7 modifies .lintr configuration to enable non-default linters
• 84edd71 to de657ed fix lints from one specific linter at a time
• 179a4de modifies .lintr configuration to disable linters that cause too many false positives or clash with design choices made in this repo (e.g. indents in function argument definition)

If all goes well, only one lint should be remaining. It flags the fact that the no_contrasts variable is not used enw_formula(). I'm not completely sure what the right fix is here. Whether no_contrasts should just be removed here or if the wrong variable is passed at some point in place of no_contrasts.

The other changes should mostly result in a more coherent styling but also in a slightly more performant code in some cases (5d7595f, d2e57ea, 4ae9ed2, 2906dc3 among others).

[pearsonca]

General question to correct my own ignorance: is there a way for me to run this check locally? As in, before committing/pushing, a one-line-at-prompt, use this configuration to lint?

[Bisaloo]

Very interesting question @pearsonca! I've never really considered it.

I usually just run lintr::lint_package(), which will automatically pick up the .lintr config file. But if you want to run this specifically on changed files, there must be a way to pipe the output from git diff (or from one of the git wrapper R package).

[Bisaloo]

Not one line but the code from the GHA can be modified to work locally on changed files, before you add them.

changed_files <- gert::git_diff()$new
all_files <- list.files(recursive = TRUE)
exclusions_list <- as.list(setdiff(all_files, changed_files))
lintr::lint_package(exclusions = exclusions_list)

[codecov]

Codecov Report

Merging #220 (440af8a) into develop (4c760f6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 440af8a differs from pull request most recent head d704ce3. Consider uploading reports for the commit d704ce3 to get more accurate results

@@           Coverage Diff            @@
##           develop     #220   +/-   ##
========================================
  Coverage    97.30%   97.31%           
========================================
  Files           14       14           
  Lines         1524     1527    +3     
========================================
+ Hits          1483     1486    +3     
  Misses          41       41           

Impacted Files	Coverage Δ
R/epinowcast.R	97.50% <ø> (ø)
R/simulate.R	100.00% <ø> (ø)
R/check.R	100.00% <100.00%> (ø)
R/formula-tools.R	95.23% <100.00%> (+0.03%)	⬆️
R/model-design-tools.R	100.00% <100.00%> (ø)
R/model-module-helpers.R	97.10% <100.00%> (-0.05%)	⬇️
R/model-modules.R	96.67% <100.00%> (ø)
R/model-tools.R	99.20% <100.00%> (+0.01%)	⬆️
R/model-validation.R	95.45% <100.00%> (ø)
R/postprocess.R	100.00% <100.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Bisaloo]

I again forgot to base my changes on develop 😢

[seabbs]

You are a 🦸🏻‍♂️! Just going through commit by commit.

I again forgot to base my changes on develop 😢

This is super easy to do. Any thoughts on ways we can make our develop approach clearer here? We could also just drop it because we now have control releases to r-universe. Hmm will add this to the docket for the next community call.

If all goes well, only one lint should be remaining. It flags the fact that the no_contrasts variable is not used enw_formula(). I'm not completely sure what the right fix is here. Whether no_contrasts should just be removed here or if the wrong variable is passed at some point in place of no_contrasts.

This is really showing the value of these additional linters! I think that the right variables are being passed (on line 667) to the underlying function as expected and that this was just an oversight (which I would hope tests would catch if causing issues). This is quite important functionality though so I think perhaps we should not change this in this PR and deal with in a separate issue just to make doubly sure that everything is working as expected.

toString

Pretty excited about this!

General question to correct my own ignorance: is there a way for me to run this check locally? As in, before committing/pushing, a one-line-at-prompt, use this configuration to lint?

@pearsonca I think precommit might do a lot of what you want here using git hooks. act for running actions locally using docker is probably exactly what you want and has worked fairly well for me (though needs to be manually triggered).

[pearsonca]

re basing off develop, is there a way to set the default base on the make-a-branch-from-this-issue clicky bits?

[seabbs]

re basing off develop, is there a way to set the default base on the make-a-branch-from-this-issue clicky bits?

I have ineffectually looked for this a few times as it seems like there must be.

[seabbs]

This all looks good to me and is a lot of work so thanks again @Bisaloo. The only outstanding actions we need before we can go ahead is to iterate the development version, and add a news item linking to this PR. We should also open an issue to address the no_contrasts linting warning but I am happy to do that.

@pearsonca or @adrian-lison (or anyone else @epinowcast/developers ) who has a chance it would be great to get a second pair of eyes on this just to check nothing has slipped through as it is a lot of changes across the code base.

[seabbs]

We should also open an issue to address the no_contrasts linting warning but I am happy to do that.

I have just made this issue and happy to merge this in with a failing lint check so it can be resolved there instead.

[Bisaloo]

to iterate the development version

Making sure I understand: should I increment to package version number to 0.2.0.7000?

What is the preferred way to resolve conflicts? Should I rebase locally and force push or should I create a merge commit here?

[seabbs]

Making sure I understand: should I increment to package version number to 0.2.0.7000?

(yes though now it is at .8000 - I read somewhere this was a good idea and we decided to try it out but it is a real pain)

What is the preferred way to resolve conflicts? Should I rebase locally and force push or should I create a merge commit here?

Whatever you would like with a slight preference for rebasing (because it is trendy). As we are using squash merging (😱) I am generally not hugely worried about PR commits as long as they are clear.

[seabbs]

This looks good to go to me. Happy to see this merged in when you are ready @Bisaloo. Thanks again for the nice clean PR.

[Bisaloo]

Happy to see this merged in when you are ready @Bisaloo.

I think it's good to go. I can't merge it myself as one check is failing but feel free to do it whenever convenient.

[seabbs]

This still all looks good to me so going to merge without a second review. Note this will cause lint checks on develop to fail but we have an issue open to resolve.