-
Notifications
You must be signed in to change notification settings - Fork 6
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
Set up lintr continuous integration and fix lints #58
Conversation
Since they can return a logical of length > 1 which will produce an error in if Reported by lintr::vector_logic_linter()
To deal with the case when x is 0 Reported by lintr::seq_linter()
Recommended by lintr::numeric_leading_zero_linter()
Recommended by lintr::T_and_F_symbol_linter()
Recommended by lintr::boolean_arithmetic_linter()
For better performance because log() is vectorized Recommended by lintr::inner_combiner_linter()
Recommended by lintr::unnecessary_concatenation_linter()
Recommended by lintr::paste_linter()
Recommended by lintr::brace_linter()
Recommended by lintr::object_usage_linter()
Thanks for the suggestions, @Bisaloo ! I'll look at this in detail over the next week or so. I'd be happy to make changes for the first two "controversial" bullet points. I worry that the third might lead to extensive suggested refactoring of code, which I don't have time to take on now. But I suppose having a report of where suggested changes might be could be useful, and a source of future development. If you have time/interest, maybe you could have put those suggestions into a separate PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the whole I really like the idea of using lintr here, and appreciate these many "clean-up" changes. The only ones that I am not sure about accepting are the ones that I commented on, where some "unused arguments" are deleted. I think those arguments (part of the error and warning functions defined as part of tryCatch() were put there to assist with debugging. I don't have the time right now to dig in and recreate a situation where they'd be used to understand it all better. But I wonder if for now we could not include those changes, and is it possible to add those suggestions to a lintr-ignore file or equivalent?
Or, if I'm completely misunderstanding how those arguments are used in the code (it's probably been over 10 years since I wrote that code) then I'd be open to hear the case to just remove them.
This PR adds lintr to the existing CI infrastructure. Changes submitted by PR will now be checked by lintr. Please note that only changed files will be checked as issues in other parts of the code are probably not the responsibility of the PR author.
I recommend reviewing this PR commit by commit as each commit focuses on fixing one specific class of lint.
The lintr config used is saved in the last commit and the analysis done in this PR can be reproduced by running
lintr::lint_package()
in the package root.There are a couple of extra linters I would recommend turning on but they might be more controversial or require larger changes so I left them off for now:
any_is_na_linter()
: replaceany(is.na(x))
by the much more performantanyNA(x)
. This adds a dependency on R 3.2 (note: tidyverse packages already require R 3.6)commented_code_linter()
: commented code should be deleted. Commented code are often breadcrumbs from previous versions. It make the actual current code harder to read by adding noise and often quickly becomes obsolete. It can be safely deleted because it's saved in the git history anywayscyclocomp_linter()
: some functions have a high cyclomatic complexity, meaning they are probably hard to understand and to maintain and are more likely to contain bugs. These functions should probably be simplified are broken into smaller, easier to understand functions. But this can sometimes require a consequent refactoring.