New warning for %between% #3015
Conversation
R/between.R
Outdated
| "%between%" <- function(x, y) { | ||
| if (length(y) > 2L) { | ||
| ysub = substitute(y) | ||
| warning("RHS has length() greater than 2. ", |
There was a problem hiding this comment.
à la if (1:5 > 1) "a" we should indicate that part of the input is being ignored: "RHS has length > 2 and only the first and second elements will be used".
There was a problem hiding this comment.
I thought to include that, but didn't want an overlong message; you don't think that's strongly implied by the warning here?
Maybe:?
RHS has length() greater than 2 and the rest will be ignored. [...]
There was a problem hiding this comment.
I think a problem can arise when the user says y = c(B, B + 1), 'knowing' that length(y) > 2 -- duh! It's vectorized; the warning is nothing to worry about.
On second thoughts, maybe this should be an error. Documentation is pretty clear that y must be length-2, it was just never enforced. I'm trying to think of a case where only a warning is appropriate.
The suggestion (Perhaps...) should obviously stay: that's imo the most valuable part of this PR.
There was a problem hiding this comment.
I also was struggling to think of a use case that's not subsumed by just running [1:2] on whatever your input is (and even the [1:2] cases i couldn't think of a real use case)
There was a problem hiding this comment.
A point well taken re "knowing" length(y) > 2. My writing was intended to convey/hint/poke "Hmm maybe I should check length(RHS) since I thought it was 2", but being more explicit is less prone to errors of assuming
There was a problem hiding this comment.
@HughParsonage updated
Codecov Report
@@ Coverage Diff @@
## master #3015 +/- ##
==========================================
+ Coverage 90.81% 90.82% +<.01%
==========================================
Files 61 61
Lines 11728 11736 +8
==========================================
+ Hits 10651 10659 +8
Misses 1077 1077
Continue to review full report at Codecov.
|
MichaelChirico
force-pushed
the
between_warning
branch
from
c0ab3d2
to
4ec01e7
Compare
Also change NEWS whitespace
MichaelChirico
force-pushed
the
between_warning
branch
from
4ec01e7
to
309462a
Compare
mattdowle
changed the title
Closes #3014