search

r-lib / lintr

Static Code Analysis for R
https://lintr.r-lib.org
Other
1.16k stars 184 forks source link

Make `scalar_in_linter()` configurable #2574

Closed F-Noelle closed 1 month ago

F-Noelle commented 1 month ago

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

MichaelChirico commented 1 month ago

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

F-Noelle commented 1 month ago

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

AshesITR commented 1 month ago

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 commented 1 month ago

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.