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

[R] [CI] use lintr 3.1.0 #9456

Merged
merged 1 commit into from
Aug 10, 2023
Merged

[R] [CI] use lintr 3.1.0 #9456

merged 1 commit into from
Aug 10, 2023

Conversation

jameslamb
Copy link
Contributor

{lintr} 3.1.0 came out a few weeks ago: https://github.com/r-lib/lintr/releases/tag/v3.1.0

It includes a few new linters, a bunch of bugfixes, and some new deprecation warnings. {xgboost} has already been using it, since this instalation is unpinned:

This PR proposes some changes to the R linting to better take advantage of that new version.

Fixes these warnings

1: Linter no_tab_linter was deprecated in lintr version 3.1.0. Use whitespace_linter instead.
2: Linter unneeded_concatenation_linter was deprecated in lintr version 3.1.0. Use unnecessary_concatenation_linter instead.

And these errors

[[1]]
R-package/demo/interaction_constraints.R:47:25: warning: [lengths] Use lengths() to find the length of each element in a list.
  interaction_length <- sapply(interaction_list, length)
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[[2]]
R-package/R/xgb.model.dt.tree.R:90:7: warning: [boolean_arithmetic] Use any() to express logical aggregations. For example, replace length(which(x == y)) == 0 with !any(x == y).
      sum(grepl('leaf=(\\d+)', text)) < 1) {
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[[3]]
R-package/R/xgb.plot.deepness.R:139:24: warning: [lengths] Use lengths() to find the length of each element in a list.
    data.table(Depth = sapply(paths, length), ID = To[Leaf == TRUE])
                       ^~~~~~~~~~~~~~~~~~~~~

[[4]]
R-package/tests/testthat/test_helpers.R:192:18: warning: [matrix] Use rowSums(shapi) rather than apply(shapi, 1, sum)
    expect_equal(apply(shapi, 1, sum), pred, tol = tol)
                 ^~~~~~~~~~~~~~~~~~~~

Notes for Reviewers

We made similar changes a few weeks ago in LightGBM and it's been working well so far: microsoft/LightGBM#5997

Thanks for your time and consideration!

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

Nice upgrade! Love watching the code being cleaned up.

@trivialfis trivialfis merged commit 4359356 into dmlc:master Aug 10, 2023
@jameslamb jameslamb deleted the r/lintr-3.1.0 branch August 10, 2023 13:49
@jameslamb
Copy link
Contributor Author

Sure, happy to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants