Apply all but some linters

[MichaelChirico]

This will make it easier to apply the panoply of linters we author to ourselves without having to add them to a growing list one-by-one.

[codecov-commenter]

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (a9659c2) 99.40% compared to head (96526d2) 99.13%.

❗ Current head 96526d2 differs from pull request most recent head c855526. Consider uploading reports for the commit c855526 to get more accurate results

Files	Patch %	Lines
R/lintr-deprecated.R	28.57%	20 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2328      +/-   ##
==========================================
- Coverage   99.40%   99.13%   -0.27%     
==========================================
  Files         124      124              
  Lines        5561     5574      +13     
==========================================
- Hits         5528     5526       -2     
- Misses         33       48      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[AshesITR]

all_linters() accepts customizations directly via ....

[MichaelChirico]

all_linters() accepts customizations directly via ....

TIL. I admit it looks strange to me this way. But may as well use the feature we've written.

[MichaelChirico]

Oh, I see, we need to supply packages=? That feels even stranger. Should we move packages= after ... in the all_linters() signature?

[AshesITR]

Sounds good. Any usages in the wild of this? I suspect not so a breaking change won't have much impact.

[MichaelChirico]

I don't see any:

https://github.com/search?q=lang%3AR%20%2Fall_linters%5C(%2F&type=code

https://github.com/search?q=path%3A.lintr+%2Fall_linters%5C%28%2F&type=code

[AshesITR]

cc @Bisaloo it seems you are using all_linters() in your work.

[MichaelChirico]

cc @Bisaloo it seems you are using all_linters() in your work.

In the public repos, package= is always supplied by name, so the proposed re-ordering of the signature of all_linters() would not affect you unless you have a different practice private repos/projects off GitHub.

[MichaelChirico]

Draft while #2332 is pending.

[AshesITR]

I would be interested in the performance implications of this change.

[Bisaloo]

cc @Bisaloo it seems you are using all_linters() in your work.

Thanks for pinging 🙏! As @MichaelChirico pointed out, the packages argument is named so nothing should break. FWIW, it would feel very strange to me to not name this argument no matter its position since it's an outlier compared to the following/preceding arguments.

[MichaelChirico]

I would be interested in the performance implications of this change.

Good call-out. I think it's slower, but not substantially:

system.time(lint_package())

# this branch
#    user  system elapsed 
# 306.824   4.011 311.853 

# main
#    user  system elapsed 
# 256.827   2.310 259.812 

[MichaelChirico]

Here are the timings of the slowest 16 individual linters:

           cyclocomp_linter     object_overwrite_linter         object_usage_linter          indentation_linter 
                     84.264                      19.455                       8.364                       7.711 
            backport_linter        object_length_linter          object_name_linter        package_hooks_linter 
                      7.590                       4.897                       4.432                       4.336 
               paste_linter      string_boundary_linter        keyword_quote_linter      list_comparison_linter 
                      4.322                       4.319                       3.875                       3.847 
  unnecessary_lambda_linter                brace_linter trailing_blank_lines_linter          fixed_regex_linter 
                      3.409                       3.287                       3.053                       3.019 

backport_linter() is a bit surprising. I think this is worth a few follow-ups.

[AshesITR]

20% slowdown seems harsh.

I'm surprised by object_overwrite_linter.
Maybe not worth precompiling disallowed names into the XPath? It shouldn't be slower than object_name_linter.

[MichaelChirico]

20% slowdown seems harsh.

not sure that's the right framing. many of these are linters we applied to ourselves but just aren't in the .lintr suite. I think more importantly it still runs faster than many of the other GHA checks so we won't really notice.

Eventually, parallelism can help.

[MichaelChirico]

Current timings:

# current main
#    user  system elapsed 
# 123.475   0.092 123.567 

# this branch
#    user  system elapsed 
# 183.452   0.960 184.412

Actually (still) 50% slower on this machine! But the absolute time is only 3 minutes. I think we should just merge & keep working on some performance issues going forward, if we really notice the lint GHA being a blocker we can revisit.

[AshesITR]

I agree. Are those timings from the same machine as before?

[MichaelChirico]

I agree. Are those timings from the same machine as before?

no. best guess is the faster absolute timings come from a machine with the faster dev version of {xmlparsedata} installed. Won't be able to reproduce that any time soon though.