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

seq_linter() gives the wrong message for n:1 lints #1542

Closed
MichaelChirico opened this issue Sep 20, 2022 · 2 comments · Fixed by #2099
Closed

seq_linter() gives the wrong message for n:1 lints #1542

MichaelChirico opened this issue Sep 20, 2022 · 2 comments · Fixed by #2099
Milestone

Comments

@MichaelChirico
Copy link
Collaborator

The result message should mention rev(), but currently it doesn't:

lint(text = ".N:1", linters = seq_linter())
# <text>:1:1: warning: [seq_linter] .N:1 is likely to be wrong in the empty edge case. Use seq_len(1) instead.
# .N:1
# ^~~~

It also says seq_len(1) instead of seq_len(.N)

@MichaelChirico MichaelChirico added this to the 3.0.2 milestone Sep 20, 2022
@IndrajeetPatil
Copy link
Collaborator

The same issue also holds for all other relevant functions I think:

library(lintr)

lint(text = "length(x):1", linters = seq_linter())
#> <text>:1:1: warning: [seq_linter] length(...):1 is likely to be wrong in the empty edge case. Use seq_along(...) instead.
#> length(x):1
#> ^~~~~~~~~~~

length(BOD):1
#> [1] 2 1

rev(seq_along(BOD))
#> [1] 2 1

Created on 2022-09-20 with reprex v2.0.2

We have a TODO to track this:

lintr/R/seq_linter.R

Lines 67 to 68 in a8d5d1f

# TODO: better message customization. For example, length(x):1
# would get rev(seq_along(x)) as the preferred replacement.

@MichaelChirico
Copy link
Collaborator Author

Also for viz: trying to get r-core to give me more confidence in recommending a replacement in the rev() case:

https://bugs.r-project.org/show_bug.cgi?id=18406

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants