Make `scalar_in_linter()` configurable #2574

F-Noelle · 2024-05-10T12:58:36Z

Make scalar_in_linter configurable to allow projects to define additional %in% style functions like %notin%.

R/scalar_in_linter.R

MichaelChirico · 2024-05-10T17:53:43Z

Thanks for the PR & great suggestion! Please add a NEWS item & be sure to cite yourself!

I'm also tempted to say the default should be character(), there's no particular reason to make %chin% special. WDYT @AshesITR / @IndrajeetPatil, since that would be a breaking change (require downstream to supply %chin% if they need it)? I do see a handful of users:

https://github.com/search?q=%2Fscalar_in_linter%2F+-path%3ANEWS&type=code

* Incorporate review feedback - Use glue in xpath, - Add changes to NEWS - Change default for scalar_in_linter - Make lint msg more open ended if another %in% operator was linted - Update tests to match new bahaviour

F-Noelle · 2024-05-10T23:43:08Z

I also tried to make the lint message for %in% like operators a bit more generic and updated the tests accordingly.

IndrajeetPatil · 2024-05-11T05:23:13Z

NEWS.md

@@ -54,6 +54,7 @@
 * `any_is_na_linter()` is extended to catch the unusual usage `NA %in% x` (#2113, @MichaelChirico).
 * `make_linter_from_xpath()` errors up front when `lint_message` is missing (instead of delaying this error until the linter is used, #2541, @MichaelChirico).
 * `paste_linter()`  is extended to recommend using `paste()` instead of `paste0()` for simply aggregating a character vector with `collapse=`, i.e., when `sep=` is irrelevant (#1108, @MichaelChirico).
+* `scalar_in_linter` is now configurable to allow other `%in%` like operators to be linted. `%chin%` is no longer linted by default. (@F-Noelle)


Since the default argument is NULL for this parameter, the second sentence is no longer needed here.

The linter previously did lint the %chin% in its unconfigurable state. As this is no longer the case due to the default changing to NULL this is a notable change. If there is a better way to phrase that I am all ears.

The larger point of the change you are making is allowing to configure this linter. %chin% was just your use case, and the developers who are reading this NEWS item don't need to know that. For users of some other package, it would have been a different operator (say %nin%).

I personally don't use {data.table} much, so I wasn't even aware of this infix operator; as a result, this part of the NEWS item was confusing for me because I thought this was an operator in base that I didn't know about.

So you might misunderstand something here. %chin% was never my use case. I know of {data.tables}, but never used it in any of my projects. I just through it would be easy to make this linter configurable to work with my use cases, hence the pull request.

Previously the linter in its unconfigurable form linted %in% and %chin%, nothing else. When I made the update I wanted to make it backwards compatible to its original use case, so I opted to make %chin% the default for the new parameter.

@MichaelChirico then suggested to demote %chin% from its special status making this PR a breaking change, which I am completely on board with. But having a breaking change is NEWS worthy in my opinion, because people might have relied on the old behaviour aka %chin% being linted.

As this is no longer the case I want to highlight that simple fact as part of the NEWS. Just because I don't rely on it doesn't mean that nobody else did. If there is a better way of phrasing this I am all ears.

I have nothing more to add here over my previous reply.

I will let @MichaelChirico decide the best phrasing for this news item, if he thinks it merits being phrased differently.

If we do make a breaking change, the corresponding bullet should be in its own section to highlight the change more clearly.

I agree with @F-Noelle about the NEWS item, it looks good to me. Maybe we can emphasize better that it's a {data.table} operator.

@IndrajeetPatil see here, I'm not quite sure your point:

lintr/R/scalar_in_linter.R

Line 35 in fc2268f

//SPECIAL[text() = '%in%' or text() = '%chin%']

On current main, x %chin% 'a' will lint

On this PR, x %chin% 'a' will only lint under scalar_in_linter("%chin%") or similar.

tests/testthat/test-scalar_in_linter.R

AshesITR · 2024-05-14T05:19:56Z

tests/testthat/test-scalar_in_linter.R


  expect_lint("x %in% 1", lint_in_msg, linter)
  expect_lint("x %chin% 'a'", lint_chin_msg, linter)
+  expect_lint("x %notin% 1", lint_notin_msg, linter)


Add a negative test case (no lints) here?

I added a test where I add negative test cases based on configuration and negative test cases for the additional in operators in general.

Is there by the way an expect_no_lint function? I would have expected a wrapper for expect_lint(content, NULL, ...).

Great idea! #2580

AshesITR · 2024-05-14T05:21:11Z

NEWS.md

@@ -54,6 +54,7 @@
 * `any_is_na_linter()` is extended to catch the unusual usage `NA %in% x` (#2113, @MichaelChirico).
 * `make_linter_from_xpath()` errors up front when `lint_message` is missing (instead of delaying this error until the linter is used, #2541, @MichaelChirico).
 * `paste_linter()`  is extended to recommend using `paste()` instead of `paste0()` for simply aggregating a character vector with `collapse=`, i.e., when `sep=` is irrelevant (#1108, @MichaelChirico).
+* `scalar_in_linter` is now configurable to allow other `%in%` like operators to be linted. `%chin%` is no longer linted by default. (@F-Noelle)


If we do make a breaking change, the corresponding bullet should be in its own section to highlight the change more clearly.

AshesITR · 2024-05-14T05:26:15Z

Thanks for the PR & great suggestion! Please add a NEWS item & be sure to cite yourself!

I'm also tempted to say the default should be character(), there's no particular reason to make %chin% special. WDYT @AshesITR / @IndrajeetPatil, since that would be a breaking change (require downstream to supply %chin% if they need it)? I do see a handful of users:

https://github.com/search?q=%2Fscalar_in_linter%2F+-path%3ANEWS&type=code

I would like to see a rough idea of the impact, using our branch comparison script for example. Slightly leaning towards a non-breaking change because false negatives (missing lints for %chin%) are generally hard to make out so lintr users basically have to read the news to find out they need to change their config.

F-Noelle · 2024-05-14T08:05:22Z

Thanks for the PR & great suggestion! Please add a NEWS item & be sure to cite yourself!
I'm also tempted to say the default should be character(), there's no particular reason to make %chin% special. WDYT @AshesITR / @IndrajeetPatil, since that would be a breaking change (require downstream to supply %chin% if they need it)? I do see a handful of users:
https://github.com/search?q=%2Fscalar_in_linter%2F+-path%3ANEWS&type=code

I would like to see a rough idea of the impact, using our branch comparison script for example. Slightly leaning towards a non-breaking change because false negatives (missing lints for %chin%) are generally hard to make out so lintr users basically have to read the news to find out they need to change their config.

I mean we could make the "%chin%" the default for in_operators again and push the change of defaults to a later point. Essentially saying the current default is "%chin%" but it is planned to switch it to NULL in the future so please make this explicit in your defaults if you rely on this functionality.

NEWS.md

R/scalar_in_linter.R

tests/testthat/test-scalar_in_linter.R

MichaelChirico · 2024-05-21T04:49:22Z

NEWS.md

@@ -54,6 +54,7 @@
 * `any_is_na_linter()` is extended to catch the unusual usage `NA %in% x` (#2113, @MichaelChirico).
 * `make_linter_from_xpath()` errors up front when `lint_message` is missing (instead of delaying this error until the linter is used, #2541, @MichaelChirico).
 * `paste_linter()`  is extended to recommend using `paste()` instead of `paste0()` for simply aggregating a character vector with `collapse=`, i.e., when `sep=` is irrelevant (#1108, @MichaelChirico).
+* `scalar_in_linter` is now configurable to allow other `%in%` like operators to be linted. The data.table operator `%chin%` is no longer linted by default; use `in_operators = "%chin%"` to continue linting it. (@F-Noelle)


sorry, i'm having trouble making edits myself since this is made in your main branch, I think you need to create a dedicated branch for me to make edits.

all this to say -- could you please move this item to the "breaking changes" section at the top :)

R/scalar_in_linter.R

MichaelChirico · 2024-05-21T04:58:18Z

R/scalar_in_linter.R

-      paste0("Use == to match length-1 scalars, not ", in_op, ". Note that == preserves NA where ", in_op, " does not.")
+    lint_msg <- glue(
+      "Use comparison operators (e.g. ==, !=, etc.) to match length-1 scalars instead of {in_op} ",
+      "as these operators preserve NA where {in_op} does not."


Sorry, I'm afraid we've over-corrected 😄

"as" implies that preserving NA is the reason for the lint. But that's not true... actually this is a somewhat ambiguous thing. Some authors might prefer this behavior and thus disfavor this linter -- hence we're highlighting this aspect as a tradeoff users should be aware of.

So let's go with

Use comparison operators... to match length-1 scalars instead of {in_op}. Note that comparison operators preserve NA where {in_op} does not.

Co-authored-by: Michael Chirico <[email protected]>

MichaelChirico

Looks good, thanks for your patience!

Update scalar_in_linter.R (#1)

60869ce

Make scalar_in_linter configurable to allow projects to define additional %in% style functions like %notin%.

F-Noelle changed the title ~~Update scalar_in_linter.R (#1)~~ Update scalar_in_linter.R May 10, 2024

MichaelChirico reviewed May 10, 2024

View reviewed changes

R/scalar_in_linter.R Outdated Show resolved Hide resolved

MichaelChirico reviewed May 10, 2024

View reviewed changes

R/scalar_in_linter.R Outdated Show resolved Hide resolved

MichaelChirico reviewed May 10, 2024

View reviewed changes

R/scalar_in_linter.R Outdated Show resolved Hide resolved

Incorporate review feedback (#2)

b38f100

* Incorporate review feedback - Use glue in xpath, - Add changes to NEWS - Change default for scalar_in_linter - Make lint msg more open ended if another %in% operator was linted - Update tests to match new bahaviour

IndrajeetPatil reviewed May 11, 2024

View reviewed changes

tests/testthat/test-scalar_in_linter.R Outdated Show resolved Hide resolved

IndrajeetPatil changed the title ~~Update scalar_in_linter.R~~ Make scalar_in_linter() configurable May 11, 2024

Add a vector of in operators (#3)

39a9c02

AshesITR reviewed May 14, 2024

View reviewed changes

Merge branch 'main' into main

ae58e20

Add a testcase based on configuration (#4)

7bd8324

MichaelChirico reviewed May 20, 2024

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

MichaelChirico reviewed May 20, 2024

View reviewed changes

R/scalar_in_linter.R Outdated Show resolved Hide resolved

MichaelChirico reviewed May 20, 2024

View reviewed changes

R/scalar_in_linter.R Outdated Show resolved Hide resolved

MichaelChirico reviewed May 20, 2024

View reviewed changes

tests/testthat/test-scalar_in_linter.R Outdated Show resolved Hide resolved

MichaelChirico mentioned this pull request May 20, 2024

New function expect_no_lint() akin to expect_no_warning() etc #2580

Closed

F-Noelle added 3 commits May 21, 2024 01:22

Merge branch 'r-lib:main' into main

b756379

Improve lint message, NEWS, param documentation (#5)

3f5eb7c

Improve lint message (#6)

04881ef

MichaelChirico reviewed May 21, 2024

View reviewed changes

R/scalar_in_linter.R Outdated Show resolved Hide resolved

MichaelChirico reviewed May 21, 2024

View reviewed changes

F-Noelle and others added 2 commits May 21, 2024 08:43

Update R/scalar_in_linter.R

49c453e

Co-authored-by: Michael Chirico <[email protected]>

Update lint message (#7)

1aa6491

MichaelChirico approved these changes May 21, 2024

View reviewed changes

MichaelChirico merged commit e97be9b into r-lib:main May 21, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `scalar_in_linter()` configurable #2574

Make `scalar_in_linter()` configurable #2574

F-Noelle commented May 10, 2024

MichaelChirico commented May 10, 2024 •

edited

Loading

F-Noelle commented May 10, 2024

IndrajeetPatil May 11, 2024

F-Noelle May 11, 2024

IndrajeetPatil May 11, 2024

F-Noelle May 11, 2024

IndrajeetPatil May 11, 2024

AshesITR May 14, 2024

MichaelChirico May 20, 2024

AshesITR May 14, 2024

F-Noelle May 14, 2024

MichaelChirico May 20, 2024

AshesITR May 14, 2024

AshesITR commented May 14, 2024

F-Noelle commented May 14, 2024

MichaelChirico May 21, 2024

F-Noelle May 21, 2024

MichaelChirico May 21, 2024

F-Noelle May 21, 2024

MichaelChirico left a comment

Make scalar_in_linter() configurable #2574

Make scalar_in_linter() configurable #2574

Conversation

F-Noelle commented May 10, 2024

MichaelChirico commented May 10, 2024 • edited Loading

F-Noelle commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshesITR commented May 14, 2024

F-Noelle commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelChirico left a comment

Choose a reason for hiding this comment

Make `scalar_in_linter()` configurable #2574

Make `scalar_in_linter()` configurable #2574

MichaelChirico commented May 10, 2024 •

edited

Loading