Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should lint_package() error on CI? #1226

Open
hadley opened this issue May 25, 2022 · 7 comments · May be fixed by #1227
Open

Should lint_package() error on CI? #1226

hadley opened this issue May 25, 2022 · 7 comments · May be fixed by #1227

Comments

@hadley
Copy link
Member

hadley commented May 25, 2022

Currently, https://github.com/r-lib/lintr/blob/main/.github/workflows/lint.yaml doesn't actually do anything because lint_package() never errors. Should it? Or should there be another function that's designed specifically for CI scenarios?

@hadley
Copy link
Member Author

hadley commented May 25, 2022

Maybe a custom function would be better — then you could generate markdown for https://github.blog/2022-05-09-supercharging-github-actions-with-job-summaries/ that provided clickable links that took you directly to the problem.

@hadley
Copy link
Member Author

hadley commented May 25, 2022

Markdown report could just be third option in print.lints

@AshesITR
Copy link
Collaborator

We currently produce check annotations, and support error_on_lint to exit if a lint is found.
Maybe we can default error_on_lint to in_ci()?

@MichaelChirico
Copy link
Collaborator

@hadley PTAL at this repo's current GHA set-up. We recently updated lint-changed-files so that the action fails if new lints are detected. That's working pretty well for us:

run: |
Sys.setenv("LINTR_ERROR_ON_LINT" = TRUE)
files <- gh::gh("GET https://api.github.com/repos/r-lib/lintr/pulls/${{ github.event.pull_request.number }}/files")
changed_files <- purrr::map_chr(files, "filename")
all_files <- list.files(recursive = TRUE)
exclusions_list <- as.list(setdiff(all_files, changed_files))
lintr::lint_package(exclusions = exclusions_list)

Please help to close if you agree that's a good approach

@hadley
Copy link
Member Author

hadley commented Oct 17, 2022

Yeah, that looks like a good pattern. Do you plan to wrap that up a bit more into a reusable function?

@MichaelChirico
Copy link
Collaborator

do you have in mind an exported lint_repo() function? or an argument to lint_package()/lint_dir()

my first instinct was just better documentation (esp. if our workflow is added as a template in r-lib/actions)

@hadley
Copy link
Member Author

hadley commented Oct 17, 2022

The advantage of making it a function is that once it gets baked into r-lib/actions it's easier for you to change the behaviour if you discover there's a better approach.

@MichaelChirico MichaelChirico linked a pull request Sep 15, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants