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.

Sign up for GitHub

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

Add lintr (fixes #179) #203

Closed
wants to merge 5 commits into from
Closed

Add lintr (fixes #179) #203

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c76e3ec
Add lintr, should fix #179
geraudster Oct 30, 2019
2d1ae93
Add requested changes
geraudster Oct 30, 2019
01aa854
Don't use TASK in Makefile
geraudster Nov 5, 2019
3957ff9
wip
geraudster Nov 19, 2019
942ca88
WIP Add styler + fix some styling issues
geraudster Nov 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .ci/lint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#!/bin/bash

# failure is a natural part of life
set -e

if [[ "$TASK" == "rpkg" ]]; then
R_PACKAGE_DIR=$(pwd)/r-pkg
Rscript -e "install.packages('lintr')"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since you've already added this install to .ci/setup.sh, can you remove it here?

Rscript -e "lintr::lint_package('${R_PACKAGE_DIR}')"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you agree to my suggestion in the other comment about creating a lint.R similar to the way I handled this in LightGBM, could you please use code similar to the way I set it up there?

One drawback of using lintr::lint_package() is that you are restricting yourself to only linting code in an R package . Ideally we'd like these R linting standard to apply to all R code in this repo, regardless of whether or not it is in the package.

So ideally you'd have a .ci/lint.R and then code like this in .ci/test.sh:

Rscript .ci/lint_r_code.R $(pwd)

fi

if [[ "$TASK" == "pypkg" ]]; then
echo "R task only"
fi
2 changes: 1 addition & 1 deletion .ci/setup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export JAVA_APT_PKG="oracle-java8-set-default"
# install these testing packages we need
if [[ "$TASK" == "rpkg" ]];
then
Rscript -e "install.packages(c('data.table', 'devtools', 'futile.logger', 'knitr', 'testthat', 'rmarkdown', 'uuid'), repos = 'http://cran.rstudio.com')"
Rscript -e "install.packages(c('data.table', 'devtools', 'futile.logger', 'knitr', 'testthat', 'rmarkdown', 'uuid', 'lintr'), repos = 'http://cran.rstudio.com')"
cp test-data/* r-pkg/inst/testdata/
fi

Expand Down
1 change: 1 addition & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ matrix:
- ES_VERSION=7.3.1
- TASK=rpkg
after_success:
- .ci/lint.sh
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please . add this logic to .ci/test.sh instead of introducing a new file? I would like linting issues to block PRs (i.e. cause a build error), not just get run as an after_success step.

Thank you for sharing the link to Jim Hester's recommended method for using lintr but in my opinion the fact that it spreads the linting details across so many files means it will be harder to maintain and change.

I got similar feedback on LightGBM and was convinced. Instead of .lintr and lint.sh, could you just add a .ci/lint.R (or whatever you want to call the file) and then call it with Rscript from .ci/test.sh?

- .ci/report_to_covr.sh
#################
# Python builds #
Expand Down
5 changes: 5 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ coverage_r: build_r
echo "Done calculating coverage"
open coverage.html

lint_r:
echo "Linting R source..."
TASK=rpkg .ci/lint.sh
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you come back and change this? This should be:

lint_r:
    echo "Linting R source..."
    Rscript .ci/lint.R $$(pwd)
    echo "Done linting"

So that when someone runs make lint_r, the linting code will run. The TASK thing is a special environment variable that only works in the context of how we have set up our Continuous Integration builds using .ci/test.sh.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take this into account.

I would be interested in fixing the linting checks. What do you think of using styler, so that every developer could use the same rules ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok great, thanks!

If you want to use styler as a way to address these checks, you are welcome to. But I do not want to make that project a part of this one's CI and I don't want to introduce any changes that are not directly related to passing the linters that you're proposing. So for example I don't want to see our comma-first lists changed to comma-last.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use precommit so styler will run automatically before you make any commit. If you don't like it, just skip the hook with --no-verify(see link above for details).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! Looks like an interesting project.

I would like to avoid using pre-commit hooks for this project, especially any that require installing additional software beyond simple shell scripts. This project has both Python and R components, and I do not want someone developing on the Python side to need an R environment locally, or vice versa.

I would like to minimize the barriers to entry for new contributors to the project, and I think that pushing checks into CI instead of making assumptions about contributors' local environments is one way to accomplish that goal.

Copy link

@lorenzwalthert lorenzwalthert Nov 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Then you could also use https://github.com/r-lib/ghactions and https://github.com/r-lib/actions, it has a styler example if that's preferred.

echo "Done linting"

docs_r: build_r
Rscript -e "devtools::document('r-pkg/')"
Rscript -e "pkgdown::build_site('r-pkg/')"
Expand Down
3 changes: 3 additions & 0 deletions r-pkg/.Rbuildignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,6 @@ vignettes/*\.pdf
# Stuff
.Rbuildignore
.*\.gitkeep

# Lintr
.lintr
11 changes: 11 additions & 0 deletions r-pkg/.lintr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
linters: list(
"closed_curly" = lintr::closed_curly_linter
, "infix_spaces" = lintr::infix_spaces_linter
, "long_lines" = lintr::line_length_linter(length = 120)
, "tabs" = lintr::no_tab_linter
, "open_curly" = lintr::open_curly_linter
, "spaces_inside" = lintr::spaces_inside_linter
, "spaces_left_parens" = lintr::spaces_left_parentheses_linter
, "trailing_blank" = lintr::trailing_blank_lines_linter
, "trailing_white" = lintr::trailing_whitespace_linter
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks! I know that in #179 I mentioned this set of linters, but since that issue was created lintr has released lintr 2.0.0 to CRAN with a bunch of new ones!

Could you update the list of linters to this?

LINTERS_TO_USE <- list(
    "assignment" = lintr::assignment_linter
    , "closed_curly" = lintr::closed_curly_linter
    , "equals_na" = lintr::equals_na_linter
    , "function_left" = lintr::function_left_parentheses_linter
    , "commas" = lintr::commas_linter
    , "concatenation" = lintr::unneeded_concatenation_linter
    , "implicit_integers" = lintr::implicit_integer_linter
    , "infix_spaces" = lintr::infix_spaces_linter
    , "long_lines" = lintr::line_length_linter(length = 120L)
    , "tabs" = lintr::no_tab_linter
    , "open_curly" = lintr::open_curly_linter
    , "paren_brace_linter" = lintr::paren_brace_linter
    , "semicolon" = lintr::semicolon_terminator_linter
    , "seq" = lintr::seq_linter
    , "single_quotes" = lintr::single_quotes_linter
    , "spaces_inside" = lintr::spaces_inside_linter
    , "spaces_left_parens" = lintr::spaces_left_parentheses_linter
    , "todo_comments" = lintr::todo_comment_linter
    , "trailing_blank" = lintr::trailing_blank_lines_linter
    , "trailing_white" = lintr::trailing_whitespace_linter
    , "true_false" = lintr::T_and_F_symbol_linter
)

1 change: 1 addition & 0 deletions r-pkg/DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ Imports:
Suggests:
covr,
knitr,
lintr,
rmarkdown,
testthat
License: BSD_3_clause + file LICENSE
Expand Down