Skip to content

Implement all_linters() wrapper to access all available linters #1854

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

Merged
merged 19 commits into from
Dec 19, 2022

Conversation

IndrajeetPatil
Copy link
Collaborator

Closes #1843

@AshesITR
Copy link
Collaborator

Can we add integration with use_lintr()?

@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2022

Codecov Report

Merging #1854 (79a8402) into main (f9839b0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1854   +/-   ##
=======================================
  Coverage   98.86%   98.86%           
=======================================
  Files         112      112           
  Lines        4838     4839    +1     
=======================================
+ Hits         4783     4784    +1     
  Misses         55       55           
Impacted Files Coverage Δ
R/use_lintr.R 100.00% <100.00%> (ø)
R/with.R 92.64% <100.00%> (+0.10%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@IndrajeetPatil IndrajeetPatil added the feature a feature request or enhancement label Dec 17, 2022
@AshesITR
Copy link
Collaborator

We need much more thorough cross referencing and update our examples that use linters_with_tags(NULL) to use the new function.

A new user should be able to easily discover all three variants and understand how and when to use what.

@IndrajeetPatil
Copy link
Collaborator Author

We need much more thorough cross referencing and update our examples that use linters_with_tags(NULL) to use the new function.

A new user should be able to easily discover all three variants and understand how and when to use what.

Good idea.

I have

  • cross-referenced them heavily in the Roxygen docs via seealso tag
  • changed the relevant code in README
  • removed one example from linters_with_tags() docs as well
  • changed relevant examples in lintr vignette

#' - [linters] for a complete list of linters available in lintr.
#' @export
all_linters <- function(packages = "lintr", ...) {
linters_with_tags(tags = NULL, packages = packages, ...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens in the unlikely case that tags is given as a named argument?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will produce an error:

library(lintr)
all_linters(tags = "default")
#> Error in linters_with_tags(tags = NULL, packages = packages, ...): formal argument "tags" matched by multiple actual arguments

Created on 2022-12-18 with reprex v2.0.2

But I doubt anyone will do this because this assumes that the user has looked at the implementation details for this newly introduced function and figured out that all_linters() is a wrapper around linters_with_tags(), and is deliberately using this argument. But, in that case, they shouldn't be surprised by the error message they get.

Copy link
Collaborator

Choose a reason for hiding this comment

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

modify_defaults(..., defaults = linters_with_tags(tags = NULL, packages = packages) should not suffer from this bug.

@IndrajeetPatil IndrajeetPatil added this to the 3.1.0 milestone Dec 19, 2022
Comment on lines 207 to 211
```r
linters: linters_with_defaults(
defaults = linters_with_tags(tags = NULL)
defaults = all_linters()
)
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should also directly call all_linters().
Is there a section mentioning linters_with_tags()?

@IndrajeetPatil
Copy link
Collaborator Author

I will fix the vignette later today.

AshesITR
AshesITR previously approved these changes Dec 19, 2022
Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

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

Nice work, thanks!


You can include tag-based linters in the configuration file, and customize them further:

```r
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed, these config examples should be ``` yaml for consistency with ll. 77 and clearer separation of R code examples and config examples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. Done.

@IndrajeetPatil IndrajeetPatil merged commit 502752c into main Dec 19, 2022
@IndrajeetPatil IndrajeetPatil deleted the 1843_all_linters branch December 19, 2022 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement new all_linters() function to easily access all linters
3 participants