kou commented on code in PR #46686: URL: https://github.com/apache/arrow/pull/46686#discussion_r2136698233
########## r/vignettes/developers/workflow.Rmd: ########## @@ -51,31 +51,19 @@ pkgdown::build_site(preview=TRUE) ## Styling and linting -### R code - -The R code in the package follows [the tidyverse style](https://style.tidyverse.org/). On PR submission (and on pushes) our CI will run linting and will flag possible errors on the pull request with annotations. +You can automatically adjust our styling and lint by [pre-commit](https://pre-commit.com/): -To run the linter locally, install the `{lintr}` package (note, we currently use a fork that includes fixes not yet accepted upstream, see how lintr is being installed in the file [`ci/docker/linux-apt-lint.dockerfile`](https://github.com/apache/arrow/blob/main/ci/docker/linux-apt-lint.dockerfile) for the current status) and then run Review Comment: OK. Let's use `lintr::lint_package("arrow/r")`. > the core dev team generally runs `arrow/r/lint.sh` to lint R and C++ at the same time. Hmm... This PR removes `r/lint.sh`... Could you share pre-commit related issues you got? Can we fix them instead of avoiding pre-commit? > I'm not sure we can remove linux-apt-lint.dockerfile just yet. IIRC the way that's set up, it will catch lints in the R packages internal C++ code but the current `r.yml` workflow just lints the R code so removing linux-apt-lint.dockerfile could cause us to merge R C++ code with lint issues. I think we could modify the R workflow to keep the functionality though. Oh, I missed that the `r.yml` workflow also runs lintr: https://github.com/apache/arrow/blob/b630f48f8464e770341e053e2fb328bd857bf72e/.github/workflows/r.yml#L334-L351 (BTW, I didn't know that `lintr::expect_lint_free()` exists. I changed the pre-commit configuration to use `lintr::expect_lint_free()` and removes `r/tools/lint.R`.) I want to remove the "Run lintr" step too because it's already done by pre-commit https://github.com/apache/arrow/blob/b630f48f8464e770341e053e2fb328bd857bf72e/.github/workflows/dev.yml#L66-L68 and `archery lint` https://github.com/apache/arrow/blob/b630f48f8464e770341e053e2fb328bd857bf72e/.github/workflows/dev.yml#L71-L78 . And pre-commit and `archery lint` check not only R code but also R C++ code. So we will not merge R C++ code with lint issues. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
