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] resolve line_length_linter warnings #8565

Merged
merged 8 commits into from Dec 14, 2022

Conversation

jameslamb
Copy link
Contributor

Contributes to #8012 .

This fixes warnings about long lines in the project's R code. It fixes 1013 of the 1285 warnings {lintr} is currently raising (link to recent CI build I got that value from).

I chose an arbitrary value (150 characters) that could be achieved without needing to touch too many lines. I'm proposing that this be accepted to at least prevent any longer lines from making it into the project's R code.

Reducing this ceiling to, say, 125 or 100 characters would involve touching a LOT more of the project's R code, and I think that should be done by maintainers if you want to enforce a stricter limit.

How I tested this

Ran the following to re-generate the docs and double-check that adding {} and changing indentation in roxygen comments wouldn't actually change their formatting.

cd R-package
R CMD INSTALL --with-keep.source .
Rscript -e "roxygen2::royxgenize(load = 'installed')"
Rscript -e "?xgboost::xgb.train"

@trivialfis
Copy link
Member

I chose an arbitrary value (150 characters) that could be achieved without needing to touch too many lines.

I don't want to go into a debate on the subject of the "optimal number of characters a line should have" and I usually just stick to the linter's default. :-) But for the XGBoost R package, I think your choice of threshold makes sense and will improve the existing codebase.

@trivialfis trivialfis merged commit 7a07dcf into dmlc:master Dec 14, 2022
@jameslamb jameslamb deleted the r/line-length branch December 14, 2022 13:04
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.

None yet

2 participants