Skip to content

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

4 participants