-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Cleaning lints across easystats packages #334
Comments
I have also updated So, if you want, you can also run the linters locally ( |
Note that you'll need the development version ( |
Yes, as a good practice, always work with the development version of |
This lintr-expression |
You need the dev version of |
Daniel has already removed that particular linter from config files. |
Short question, what does the "test-example-coverage" workflow do? It's always failing, and I can't see the benefits from this workflow. |
See #331 for full details. |
Okay, so in order to make this task more manageable, we can rely on The Boy Scout Rule:
A way to do this would be to clean lints only in the files that have changed in a given PR. For example, if a given repo has over 200 files, but the PR has changed only 5 files, the GHA workflow for linting will fail only if the lints in these 5 files have not been cleaned. I think this is a much more manageable task than cleaning lints across all 200 files. If we do this for a few months, we should have made a significant headway on this task. So, if you make a PR, watch for the build failures due to Once again, if you can't figure out why something is producing a lint or how to get rid of it, let me know. Don't remove the workflow to make your life easier. That's just going to lead to us acquiring more technical debt. |
this one is to torture @DominiqueMakowski |
|
@rempsyc is on fire! 🔥🔥🔥 Suddenly, |
* replace `requiet()` by `skip_if_not_installed()` + `::` * replace `.runThisTest` by `skip_on_cran()` * style and fix a few errors * clean `tests/testthat.R` * style + clean messages * fix errors with R CMD check strict * fix some errors in tests * add missing packages in "Suggests" * add missing `skip_if_not_installed()` * clean a few lints related to `expect_equal()` cf. easystats/easystats#334 * clean `line_length_linter()` lints cf. easystats/easystats#334 * fix two failing tests * fix styling workflow * fix options for message on exponentiating coeffs * don't skip survival test * fix some messages / errors * fix some messages / errors * silence some messages * silence some messages, fix a few TODOs * add missing snapshot * fix styling workflow * fix new test added in the main branch * fix some errors and messages * fixes? --------- Co-authored-by: Indrajeet Patil <patilindrajeet.science@gmail.com> Co-authored-by: Daniel <mail@danielluedecke.de>
Preamble
Lints are code quality issues that can be detected in R using
{lintr}
and need to be either fixed (if it's really an issue) or annotated so that they are ignored. We detect lints using GHA workflow, and every package includes this workflow. Static code analysis is not always correct, esp. in edge cases, and so you should also see if the lints false positives. If yes, let me know, and I can report and try to fix this in lintr itself.Example
What does lint output look like?
For an example,
{insight}
lint workflow currently shows the following lint (alongside many others):The code in question is:
As the lint message says, this code can be rewritten to the equivalent and more efficient version:
Once you make this change, the lint workflow will no longer flag these lines of code.
Help wanted
Although Daniel and I have been cleaning lints once in a while, this requires a much more concerted effort from the larger team since there are a ton of lints to be cleaned across repos.
I am tagging this issue with "Help wanted" and "Beginner-friendly" labels, hoping that we might get some community help to do this clean-up.
The text was updated successfully, but these errors were encountered: