Skip to content
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

scalar_in_linter() for x %in% 1 #2084

Merged
merged 14 commits into from
Sep 7, 2023
Merged

scalar_in_linter() for x %in% 1 #2084

merged 14 commits into from
Sep 7, 2023

Conversation

MichaelChirico
Copy link
Collaborator

Part of #884

@AshesITR
Copy link
Collaborator

AshesITR commented Aug 14, 2023

In the presence of NAs, x %in% "a" is much better than !is.na(x) & x == "a" IMO...

@AshesITR
Copy link
Collaborator

We could lint x %in% NA though to suggest is.na(x).

@MichaelChirico
Copy link
Collaborator Author

In the presence of NAs, x %in% "a" is much better than !is.na(x) & x == "a" IMO...

I prefer the latter for making missing behavior explicit... if you prefer the former it's best not to use this linter I'd say. We can tweak the documentation to reflect that it may be up to taste, WDYT?

@MichaelChirico
Copy link
Collaborator Author

We could lint x %in% NA though to suggest is.na(x).

I'd make that a message customization as a follow-up

@AshesITR
Copy link
Collaborator

In the presence of NAs, x %in% "a" is much better than !is.na(x) & x == "a" IMO...

I prefer the latter for making missing behavior explicit... if you prefer the former it's best not to use this linter I'd say. We can tweak the documentation to reflect that it may be up to taste, WDYT?

Fine by me, though I might turn on the RHS = NA case specifically.

@AshesITR
Copy link
Collaborator

We could lint x %in% NA though to suggest is.na(x).

I'd make that a message customization as a follow-up

Keep in mind that will generate incorrect advice since the behavior of == differs from %in% there.
It would be nice to restrict this linter to only this case btw because x %in% NA vs. is.na(x) is more clearly a lint IMO.
The logic should be quite simple - extract the constant as an R value and test it using is.na() or just hard-code the possible NAs, maybe offering capabilities for extension to NAs from other packages such as lubridate::NA_Date_.

@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2023

Codecov Report

Merging #2084 (c3f43be) into main (35f3344) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head c3f43be differs from pull request most recent head 170d217. Consider uploading reports for the commit 170d217 to get more accurate results

@@           Coverage Diff           @@
##             main    #2084   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files         118      119    +1     
  Lines        5210     5227   +17     
=======================================
+ Hits         5192     5209   +17     
  Misses         18       18           
Files Changed Coverage Δ
R/scalar_in_linter.R 100.00% <100.00%> (ø)

inst/lintr/linters.csv Show resolved Hide resolved
R/scalar_in_linter.R Outdated Show resolved Hide resolved
@MichaelChirico MichaelChirico merged commit bfff3d9 into main Sep 7, 2023
21 checks passed
@MichaelChirico MichaelChirico deleted the scalar_in branch September 7, 2023 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants