Lintr step should fail by defaullt if a violation is found

[edmondop]

The current lintr workflows for example here https://github.com/r-lib/actions/blob/v2-branch/examples/lint.yaml doesn't fail if a violation is found, which is counterintuive and differs from what normally CI/CD workflows do.

I think we should fail by default, and let people "ignore" the failure with an option. Unfortunately the guide here https://github.com/r-lib/lintr#continuous-integration is not very clear, I would have expected this to be the default

[IndrajeetPatil]

I think we should fail by default, and let people "ignore" the failure with an option

IMHO, this will be a bad idea.

There can often be a few false positives (e.g. function name should be snake_case lint for generic method) in lintr. So, if GHA defaults to failure for any violation found, unfortunately, this will just incentivize people to remove lintr GHA.

cc @MichaelChirico

[edmondop]

"Tests can be flaky, so don't run tests in CI"

[IndrajeetPatil]

Linting is not unit testing. They are good suggestions for improving code quality, but they are not 100% correct.

[gaborcsardi]

There is nothing wrong with having an option for it.

[IndrajeetPatil]

There is nothing wrong with having an option for it.

Sure, no doubt about that.

But I am just pushing back against the suggested default, which is to mark build a failure if any lint is found.

A better default would be the current behaviour, and if someone wishes to, they can choose the option to mark the build failed in case any lints are found.

[MichaelChirico]

I think the default should be failure. false positives should be reported and/or # nolint'd

[MichaelChirico]

lintr itself has been bitten a few times recently from the lack of failure. it's easy to miss the warnings in a review, leading to extra review toil (1) handling follow-up PRs and (2) getting distracted in unrelated PRs by noticing the lint warnings there.

[edmondop]

💯 @MichaelChirico totally agree

[richelbilderbeek]

I am with @edmondo1984 and @MichaelChirico that failing should be the default :-)

[aronatkins]

The LINTR_ERROR_ON_LINT environment variable provides the default for the lintr option error_on_lint, which should provide the option that folks want.

We have another project that has a CI check that is effectively:

export LINTR_ERROR_ON_LINT=true
Rscript -e 'lintr::lint_dir("R")'

You should be able to set this environment variable in your existing actions and see lintr rule violations fail CI.

[gaborcsardi]

@aronatkins sounds great, thanks much!

Now the question is whether that should default to true or not? If yes, then we could set this env var in the example workflow. It seems like people in this thread would favor that.

Anyone wants to submit a PR?

[assignUser]

I just want to mention lintr::expect_lint_free

[github-actions[bot]]

This issue has been automatically locked. If you believe you have found a related problem, please file a new issue and include a link to this issue