[R-package] added R linting and changed R code to comma-first (fixes #2373) #2437

jameslamb · 2019-09-24T00:38:00Z

In this PR, I propose adding a linter for code in the R package. For now I propose just tacking this on to Travis until I figure out #2353 (I have a support case pending with GitHub support for what I think is a bug in GitHub Actions).

See more information behind this PR in #2373 .

This block in .ci/lint_r_code.R documents the key linters I've chosen to use from the lintr package.

LINTERS_TO_USE <- list(
    "open_curly" = lintr::open_curly_linter
    , "closed_curly" = lintr::closed_curly_linter
    , "tabs" = lintr::no_tab_linter
    , "spaces" = lintr::infix_spaces_linter
    , "trailing_blank" = lintr::trailing_blank_lines_linter
    , "trailing_white" = lintr::trailing_whitespace_linter
    , "long_lines" = lintr::line_length_linter(length = 120)
)

They express the following:

{ or } should never be on a line by themselves
only spaces should be used for indentation (not tabs)
infix operators should have spaces around them, so e.g. rnorm(n = 5) is preferred to rnorm(n=5)
files should end in exactly one empty line
files should never have trailing horizontal whitespace
no files should have lines longer than 120 characters

These linters are cheap to run, as they don't require installation of the package or evaluation of any of the package's code. There is no clean "always use comma-first" liner that I'm aware of for R code, so that will have to be enforced by convention.

In the way I've set this up, this will be applied to all files with extension .R or .r.

The files introduced into .ci/ are adopted from a set of scripts I maintain for re-use by various open source projects I work on (https://github.com/jameslamb/ci_tools/tree/master/r).

jameslamb · 2019-09-24T03:10:03Z

Microsoft.LightGBM and appveyor are failing with this error that I'm pretty sure is unrelated to this PR:

ImportError: No module named 'win32api'

StrikerRUS · 2019-09-24T12:10:58Z

@jameslamb

Microsoft.LightGBM and appveyor are failing with this error that I'm pretty sure is unrelated to this PR:
ImportError: No module named 'win32api'

Refer to jupyterlab/jupyterlab#7259 and conda-forge/jupyter_client-feedstock#29.

StrikerRUS

@jameslamb Thanks for your PR! Please address some comments related to setup process below.

.ci/lint_r.sh

.ci/test.sh

.ci/lint_r.sh

jameslamb · 2019-09-24T23:31:19Z

@jameslamb Thanks for your PR! Please address some comments related to setup process below.

thanks for the review! Addressed the comments + rebased to master to get the fix from #2440

StrikerRUS

@jameslamb Thanks a lot for merging linting jobs and removing excess files! I've given a more closer look than yesterday and left some comments.

Also, I guess, that .Rd files should be re-generated.

Sorry for any stupid comments related to linting itself (if any) in advance 🙂 .

R-package/R/lgb.cv.R

StrikerRUS · 2019-09-25T13:22:58Z

R-package/R/lgb.interprete.R

-                                FUN = function(x) matrix(seq_len(length(x)) - 1, ncol = num_class, byrow = TRUE))
+  tree_index_mat_list <- lapply(
+    X = leaf_index_mat_list
+    , FUN = function(x){


Space before {?

I usually do not use a space before { and the linters I'm proposing don't enforce that

R-package/R/lgb.interprete.R

R-package/R/lgb.prepare.R

R-package/R/lgb.prepare_rules2.R

StrikerRUS · 2019-09-25T14:03:07Z

.ci/lint_r_code.R

+
+issues_found <- length(results)
+
+if (issues_found > 0){


Space before {?

.ci/test.sh

.travis.yml

.ci/lint_r_code.R

StrikerRUS

@jameslamb Ah, sorry, forget to add the most important comment! In order to really run R linting script, its' call should be performed before cpplint check due to temp || exit 0 after it.

StrikerRUS · 2019-09-25T14:29:08Z

.ci/test.sh

    cpplint --filter=-build/include_subdir,-build/header_guard,-whitespace/line_length --recursive ./src ./include || exit 0
+    echo "linting R code"
+    Rscript ${BUILD_DIRECTORY}/.ci/lint_r_code.R \
+        --source-dir ${BUILD_DIRECTORY}


Please add || exit -1 after the Rscript call

We are guaranteed to get a nonzero exit code from the code I'm proposing, that is what this line of code does: https://github.com/jameslamb/LightGBM/blob/misc/r_code_style/.ci/lint_r_code.R#L64

quit(status = 10) (for example) causes an R script to exit 10

Do you want me to add the || exit -1 just as a source of documentation in test.sh?

@jameslamb

Do you want me to add the || exit -1 just as a source of documentation in test.sh?

Yes, please do this to force Travis to fail when R script returns non-zero status. As the R script is called from the shell script, without that || exit -1 Travis will continue running the rest of the shell script.

Ah I see. Sorry, not trying to be annoying about this.

Any reason we do not just set set -e at the top of test.sh? If we did that, any call exiting with a non-zero exit code would cause the script itself to exit with a non-zero and we wouldn't have to manually manage exit codes on all of these steps.

If you are open to that I'd be happy to open a separate PR that adds a set -e and removes all the || exit -1 in this script.

No problem!

Any reason we do not just set set -e at the top of test.sh?

IDK, let's try and see... Maybe we should allow something to fail.
But let's do not mix things: merge this PR with || exit -1 and then feel free to replace all || exit -1 with set -e.

🤝 agreed!

jameslamb · 2019-09-29T03:21:18Z

Also, I guess, that .Rd files should be re-generated.

Just regenerated the files. Note that to get past the r-lib/roxygen2#822, I had to install the latest roxygen2. Steps to build the docs on Mac (just documenting in case anyone is curious).

export CXX=/usr/local/bin/g++-8 CC=/usr/local/bin/gcc-8
Rscript build_r.R

# install roxygen2 from github
Rscript -e "devtools::install_github('r-lib/roxygen2')"
cd R-package
Rscript -e "roxygen2::roxygenise(load = 'installed')"

StrikerRUS · 2019-09-29T12:13:41Z

@jameslamb Something is wrong:

Linting R code
Error in library(lintr) : there is no package called ‘lintr’
Execution halted
Linting C++ code

https://travis-ci.org/microsoft/LightGBM/jobs/590986051#L838-L842

BTW, as you can see, despite the R script failed, Travis lint job is still green an didn't stopped after a failure in the R script. That is why || exit -1 is needed.

StrikerRUS · 2019-09-29T12:16:27Z

.ci/lint_r_code.R

+    , include.dirs = FALSE
+)
+
+# Some linters from the lintr package have not made it to CRAN yet


How many (approximately: 1, 2, or more) linters are not available with the package version installed from CRAN? I guess, there will be many problems with the installation from sources (perhaps, we already have them, refer to Error in library(lintr) : there is no package called ‘lintr’). So, maybe introduce new linters when they will be available on CRAN?

it's on the order of 4-6. Yeah, I kind of wanted to test how the installation from source plays with conda R, and the only way I knew to reliably reproduce the environment on Travis was to just directly try on Travis.

I'll take out the unreleased ones for now and create an issue (closed and attached to #2302 ) for adding the new ones once lintr gets updated. I'm also going to gently open a "can you please do a release" issue on lintr, since it has been almost a year since the last one.

Thank you very much! I think it's the most appropriate way to introduce the linting and do not suffer from it!

I have added a comment on r-lib/lintr#318 asking for a new release.

jameslamb · 2019-09-29T16:08:06Z

@jameslamb Something is wrong:

Linting R code
Error in library(lintr) : there is no package called ‘lintr’
Execution halted
Linting C++ code

https://travis-ci.org/microsoft/LightGBM/jobs/590986051#L838-L842

BTW, as you can see, despite the R script failed, Travis lint job is still green an didn't stopped after a failure in the R script. That is why || exit -1 is needed.

Thanks for catching that. Now that I've added the explicit exit it does now seem CI is correctly failing.

jameslamb · 2019-10-09T23:40:13Z

ping @Laurae2 for a review

(I'll rebase to master and make any code changes to pass linting once we agree on the set of linters)

.ci/test.sh

jameslamb · 2019-10-24T20:47:01Z

Thanks for the reviews everyone! I know it's just a style PR but I'm excited about this one. Making the R side of the CI stronger makes me happy 😀

jameslamb requested review from Laurae2, guolinke and StrikerRUS September 24, 2019 00:38

jameslamb mentioned this pull request Sep 24, 2019

[R-package] factored dependency 'magrittr' out of R package #2334

Merged

StrikerRUS requested changes Sep 24, 2019

View reviewed changes

StrikerRUS mentioned this pull request Sep 24, 2019

[ci] temp fix for missing dependency on Windows #2440

Merged

jameslamb force-pushed the misc/r_code_style branch from 869b43c to 73a0649 Compare September 24, 2019 23:30

jameslamb force-pushed the misc/r_code_style branch from 73a0649 to e7c0e90 Compare September 24, 2019 23:41

jameslamb changed the title ~~added R linting and changed R code to comma-first (fixes #2373)~~ [R-package] added R linting and changed R code to comma-first (fixes #2373) Sep 25, 2019

StrikerRUS requested changes Sep 25, 2019

View reviewed changes

jameslamb force-pushed the misc/r_code_style branch from e7c0e90 to 2d6a7ed Compare September 29, 2019 03:09

jameslamb force-pushed the misc/r_code_style branch from 1212e3b to 5689a5e Compare September 29, 2019 04:03

StrikerRUS reviewed Sep 29, 2019

View reviewed changes

jameslamb mentioned this pull request Sep 29, 2019

[R-package] Add additional linters to CI #2477

Closed

jameslamb force-pushed the misc/r_code_style branch from 3a7ec88 to 06dfd8c Compare September 29, 2019 16:21

jameslamb mentioned this pull request Sep 30, 2019

[R-package] Export callback functions #2479

Open

3 tasks

jameslamb force-pushed the misc/r_code_style branch from 06dfd8c to 88eeb89 Compare October 6, 2019 02:04

jameslamb mentioned this pull request Oct 6, 2019

[ci] removed need to explicitly exit from failed commands in CI #2497

Closed

jameslamb force-pushed the misc/r_code_style branch from 88eeb89 to 8d1da11 Compare October 21, 2019 22:16

Laurae2 approved these changes Oct 23, 2019

View reviewed changes

StrikerRUS requested changes Oct 23, 2019

View reviewed changes

.ci/test.sh Outdated Show resolved Hide resolved

jameslamb force-pushed the misc/r_code_style branch from 8d1da11 to b23067d Compare October 24, 2019 01:58

jameslamb added 10 commits October 23, 2019 21:27

added R linting and changed R code to comma-first (fixes microsoft#2373)

37b294b

exit from R linting task

68687b5

simplified setup for R linting

1f30204

fixed long lines and alignment of roxygen blocks

5cfb2ff

updated roxygen files

ede360b

added more R linters

83502e9

updated list of R linters

eaf4631

fixed bad object reference in linting code

1eca955

fixed cpplint bad rebase

3936709

more minor changes to style

d807aa3

jameslamb force-pushed the misc/r_code_style branch from 9dd8dd9 to d807aa3 Compare October 24, 2019 02:39

regenned roxygen documentation

14d285a

jameslamb mentioned this pull request Oct 24, 2019

Add linting on R code uptake/uptasticsearch#179

Closed

StrikerRUS approved these changes Oct 24, 2019

View reviewed changes

jameslamb merged commit fc991c9 into microsoft:master Oct 24, 2019

jameslamb mentioned this pull request Oct 30, 2019

Add lintr (fixes #179) uptake/uptasticsearch#203

Closed

jameslamb deleted the misc/r_code_style branch January 27, 2020 00:15

StrikerRUS added the maintenance label Mar 1, 2020

lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R-package] added R linting and changed R code to comma-first (fixes #2373) #2437

[R-package] added R linting and changed R code to comma-first (fixes #2373) #2437

jameslamb commented Sep 24, 2019

jameslamb commented Sep 24, 2019

StrikerRUS commented Sep 24, 2019

StrikerRUS left a comment

jameslamb commented Sep 24, 2019

StrikerRUS left a comment

StrikerRUS Sep 25, 2019

jameslamb Sep 27, 2019

StrikerRUS Sep 25, 2019

StrikerRUS left a comment

StrikerRUS Sep 25, 2019

jameslamb Sep 29, 2019

StrikerRUS Sep 29, 2019

jameslamb Sep 29, 2019

jameslamb Sep 29, 2019

StrikerRUS Sep 29, 2019

jameslamb Sep 29, 2019

jameslamb commented Sep 29, 2019

StrikerRUS commented Sep 29, 2019

StrikerRUS Sep 29, 2019 •

edited

Loading

jameslamb Sep 29, 2019

StrikerRUS Sep 29, 2019

jameslamb Sep 29, 2019

jameslamb commented Sep 29, 2019

jameslamb commented Oct 9, 2019

jameslamb commented Oct 24, 2019

[R-package] added R linting and changed R code to comma-first (fixes #2373) #2437

[R-package] added R linting and changed R code to comma-first (fixes #2373) #2437

Conversation

jameslamb commented Sep 24, 2019

jameslamb commented Sep 24, 2019

StrikerRUS commented Sep 24, 2019

StrikerRUS left a comment

Choose a reason for hiding this comment

jameslamb commented Sep 24, 2019

StrikerRUS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StrikerRUS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb commented Sep 29, 2019

StrikerRUS commented Sep 29, 2019

StrikerRUS Sep 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb commented Sep 29, 2019

jameslamb commented Oct 9, 2019

jameslamb commented Oct 24, 2019

StrikerRUS Sep 29, 2019 •

edited

Loading