Create condition_call_linter #2226

Bisaloo · 2023-10-03T20:19:03Z

I'm opening this PR so we have something tangible to discuss but I'm happy to take any feedback and do any necessary changes.

Bisaloo · 2023-10-03T20:29:47Z

As far as I can tell, tests fail because there are snapshot tests to control the addition of new defaults. It's indeed probably safer to not include this linter as default at least for now. Please let me know what you think.

AshesITR · 2023-10-04T12:20:53Z

Since it's part of the style guide I am in favor of adding it to the defaults.
The default linter test code would need to be expanded to create a lint for this linter.

Regarding the design, I'm not sure we should lint any call.= calls under the assumption that the author has put thought into the value of call..
Therefor I'd only lint stop() without a call. argument by default. The lint message could still suggest call. = FALSE.
LMK what you think about that.

MichaelChirico · 2023-10-04T16:28:39Z

Since it's part of the style guide

Is it? The 'design' book is not the style guide... the latter AFAICT is mum on the topic. Would help to know if we intend to enforce both the style guide and the design guide by default (what does {styler} do here @lorenzwalthert?)

AshesITR · 2023-10-04T17:20:40Z

R/condition_call_linter.R

+#' )
+#'
+#' @evalRd rd_tags("condition_call_linter")
+#' @seealso [linters] for a complete list of linters available in lintr.


Add a reference link to design.tidyverse.org?

I missed the location of this comment. The link is now provided here: https://github.com/Bisaloo/lintr/blob/16a9b3f021bf960fca19e91568a0cecc75f2764e/R/condition_call_linter.R#L3-L5. Please let me know if you don't think it's appropriate.

We usually put it in @seealso.

AshesITR · 2023-10-04T17:21:02Z

Since it's part of the style guide

Is it? The 'design' book is not the style guide... the latter AFAICT is mum on the topic. Would help to know if we intend to enforce both the style guide and the design guide by default (what does {styler} do here @lorenzwalthert?)

Good catch.

lorenzwalthert · 2023-10-04T19:59:18Z

{styler} only implements the style guide as per the docs.

codecov-commenter · 2023-10-05T09:37:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (edf8df2) 99.10% compared to head (134ceb0) 99.11%.

❗ Current head 134ceb0 differs from pull request most recent head 7882955. Consider uploading reports for the commit 7882955 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2226   +/-   ##
=======================================
  Coverage   99.10%   99.11%           
=======================================
  Files         124      125    +1     
  Lines        5612     5646   +34     
=======================================
+ Hits         5562     5596   +34     
  Misses         50       50

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

Bisaloo · 2023-10-05T09:43:15Z

Regarding the design, I'm not sure we should lint any call.= calls under the assumption that the author has put thought into the value of call..
Therefor I'd only lint stop() without a call. argument by default. The lint message could still suggest call. = FALSE.
LMK what you think about that.

That's a good question 🤔

I could still imagine that it's useful to ensure consistency across different contributors (or different projects using the same lintr settings). The code complexity added by linting call. seems low enough that it's worth offering this flexibility.

I don't feel super strongly on this though as I personally prefer call. = FALSE but I have heard diverging opinions on the matter. Happy to follow your recommendation.

AshesITR · 2023-10-05T10:45:39Z

Yes to offering flexibility.

What do you think about these options?

call. = NA # force providing call.=, ignore its value
call. = TRUE # lint call. = FALSE, ignore missing call.=
call. = FALSE # force call. = FALSE

Default could be FALSE as by the design guide.

Bisaloo · 2023-10-08T14:23:00Z

Any thoughts about the argument name? I have used display_call since I wasn't really inspired. I see you used call., should I update it to this?

AshesITR · 2023-10-08T14:38:53Z

I'm not sure, use_call comes to mind but that implies to me that TRUE does what NA is supposed to do and FALSE requires call. to be missing so that's not ideal, really.

@MichaelChirico, @IndrajeetPatil any thoughts?

AshesITR · 2023-10-08T14:50:37Z

Other linters tend to have some *style argument. Maybe that's also a valid idea.

Edit:

Maybe
call_style = c("always_hide", "always_show", "specify")

Bisaloo · 2023-10-10T10:30:30Z

BTW, I went back to the style guide history and I confirm this used to be part of the official style guide but is now obsolete since rlang is now recommended instead of base conditions:

tidyverse/style@78bc2e2

I'm not completely sure what makes sense in this case.

AshesITR · 2023-10-11T23:53:11Z

Let's leave it as non-default for the moment.

Bisaloo · 2023-11-28T11:20:45Z

Hi all, any action required from my end to move forward with this PR?

AshesITR · 2023-11-28T18:22:05Z

Hi, you could address my comment (adding a link to the location in tidy design principles which states what this linter checks).
There are also a few lints that need fixing and an entry in NEWS.md giving you credit should be added.

Bisaloo · 2023-11-30T17:48:59Z

How should I go about applying this linter to lintr itself? I think you usually do a separate PR. Is that right?

IndrajeetPatil · 2023-11-30T18:02:25Z

How should I go about applying this linter to lintr itself? I think you usually do a separate PR. Is that right?

No, we actually fix the lints in lintr in the PR itself. This exercise acts as a good proof of concept 😺

IndrajeetPatil · 2023-11-30T18:04:25Z

R/condition_call_linter.R

+    /parent::expr
+  ")
+
+  Linter(function(source_expression) {


You will also need to update this PR to use this new attribute introduced in #2352

MichaelChirico · 2023-11-30T21:50:24Z

How should I go about applying this linter to lintr itself? I think you usually do a separate PR. Is that right?

No, we actually fix the lints in lintr in the PR itself. This exercise acts as a good proof of concept 😺

I would say 'it depends', if there are only a few hits on {lintr}, go ahead and include fixes, but if there are dozens of things to change it will be better in a separate/focused PR.

Anyway, maybe you are asking about whether the new linter will be part of our .lintr config. As of #2328 all linters are applied to {lintr} right away by default.

Bisaloo · 2023-11-30T21:55:59Z

How should I go about applying this linter to lintr itself? I think you usually do a separate PR. Is that right?

No, we actually fix the lints in lintr in the PR itself. This exercise acts as a good proof of concept 😺

I would say 'it depends', if there are only a few hits on {lintr}, go ahead and include fixes, but if there are dozens of things to change it will be better in a separate/focused PR.

Anyway, maybe you are asking about whether the new linter will be part of our .lintr config. As of #2328 all linters are applied to {lintr} right away by default.

I've made the change in 0b2e34d and 427b258 but I can easily cherry-pick them in a different branch and remove them from here if you prefer.

MichaelChirico · 2023-12-01T05:10:24Z

How should I go about applying this linter to lintr itself? I think you usually do a separate PR. Is that right?

No, we actually fix the lints in lintr in the PR itself. This exercise acts as a good proof of concept 😺

I would say 'it depends', if there are only a few hits on {lintr}, go ahead and include fixes, but if there are dozens of things to change it will be better in a separate/focused PR.
Anyway, maybe you are asking about whether the new linter will be part of our .lintr config. As of #2328 all linters are applied to {lintr} right away by default.

I've made the change in 0b2e34d and 427b258 but I can easily cherry-pick them in a different branch and remove them from here if you prefer.

Yes, please do. The diff is quite large now.

IndrajeetPatil · 2023-12-01T05:17:25Z

Sorry, @Bisaloo, for the incorrect advice. Didn't realize there would be so many new lints found in lintr for the current linter.

Bisaloo · 2023-12-01T14:29:38Z

Yes, please do. The diff is quite large now.

There is now a separate PR for this: #2377. I'll rebase this PR on main once it's merged.

Sorry, @Bisaloo, for the incorrect advice. Didn't realize there would be so many new lints found in lintr for the current linter.

No worries, I understand there can be some variability depending on the specifics of each case and it wasn't much work to move these commits to a separate branch.

Bisaloo · 2023-12-01T17:09:17Z

This PR has now been rebased on main.

AshesITR

LGTM. I'd move the link to @seealso for consistency among the help pages, though.

AshesITR reviewed Oct 4, 2023

View reviewed changes

AshesITR mentioned this pull request Oct 4, 2023

Should design.tidyverse.org be authoritative for defaults? #2229

Closed

Bisaloo force-pushed the call-condition branch from eafc6fb to a711636 Compare November 30, 2023 17:31

IndrajeetPatil reviewed Nov 30, 2023

View reviewed changes

Bisaloo mentioned this pull request Dec 1, 2023

Add call. = FALSE to warning() & stop() calls #2377

Merged

Bisaloo added 3 commits December 1, 2023 17:50

Create condition_call_linter

60dc361

Add condition_call_linter.R

6225b80

Use tidy_design tag instead of default

ff44439

Bisaloo added 10 commits December 1, 2023 17:50

Fix tidy_design tag linter extraction

432d35f

Add display_call = NA option

523159c

Document display_call argument

4e989d6

Link to design guide relevant section

b941683

Fix lints

a244743

Remove stray link

0063c16

Add NEWS item

37c7c8a

Run devtools::document()

2efdf09

Add linter_level argument

a0054da

Handle xml_missing edge case

51a8e06

Bisaloo force-pushed the call-condition branch from 12b6b1e to 51a8e06 Compare December 1, 2023 16:50

AshesITR reviewed Dec 1, 2023

View reviewed changes

Move link to seealso

7882955

AshesITR approved these changes Dec 1, 2023

View reviewed changes

AshesITR merged commit 38c3f62 into r-lib:main Dec 1, 2023

Bisaloo deleted the call-condition branch December 2, 2023 08:55

Create condition_call_linter #2226

Create condition_call_linter #2226

Uh oh!

Conversation

Bisaloo commented Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bisaloo commented Oct 3, 2023

Uh oh!

AshesITR commented Oct 4, 2023

Uh oh!

MichaelChirico commented Oct 4, 2023

Uh oh!

AshesITR Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

Bisaloo Nov 30, 2023

Choose a reason for hiding this comment

Uh oh!

AshesITR Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

AshesITR commented Oct 4, 2023

Uh oh!

lorenzwalthert commented Oct 4, 2023

Uh oh!

codecov-commenter commented Oct 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Bisaloo commented Oct 5, 2023

Uh oh!

AshesITR commented Oct 5, 2023

Uh oh!

Bisaloo commented Oct 8, 2023

Uh oh!

AshesITR commented Oct 8, 2023

Uh oh!

AshesITR commented Oct 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bisaloo commented Oct 10, 2023

Uh oh!

AshesITR commented Oct 11, 2023

Uh oh!

Bisaloo commented Nov 28, 2023

Uh oh!

AshesITR commented Nov 28, 2023

Uh oh!

Bisaloo commented Nov 30, 2023

Uh oh!

IndrajeetPatil commented Nov 30, 2023

Uh oh!

IndrajeetPatil Nov 30, 2023

Choose a reason for hiding this comment

Uh oh!

MichaelChirico commented Nov 30, 2023

Uh oh!

Bisaloo commented Nov 30, 2023

Uh oh!

MichaelChirico commented Dec 1, 2023

Uh oh!

IndrajeetPatil commented Dec 1, 2023

Uh oh!

Bisaloo commented Dec 1, 2023

Uh oh!

Bisaloo commented Dec 1, 2023

Uh oh!

AshesITR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bisaloo commented Oct 3, 2023 •

edited

Loading

codecov-commenter commented Oct 5, 2023 •

edited

Loading

AshesITR commented Oct 8, 2023 •

edited

Loading