search

brodieG / vetr

Trust, but Verify
78 stars 2 forks source link

Should tokens that evaluate to zero-length logicals be treated as TRUE? #89

Closed franknarf1 closed 6 years ago

franknarf1 commented 6 years ago

I have a date formatted as an 8-digit integer and my first step in checking it is verifying that it has 8 digits:

vet(integer() && nchar(as.character(.)) == 8L, integer()) 
# [1] "`nchar(as.character(integer())) == 8L` is not TRUE (zero length)"

Contrary to that message, I'd say this is true for my purposes. Ideally, I'd set this behavior via a global option (as contemplated in #26) so I could continue encapsulating the desired test in the token alone. Of course, I could edit my rule to...

vet(integer() && (length(.) == 0L || nchar(as.character(.)) == 8L), integer())

But that would quickly become tedious since this isn't the only token where I'd want zero-length to be treated as true.

brodieG commented 6 years ago

I would use:

vet(integer() && all(nchar(as.character(.)) == 8L), integer())

I thought originally my intent was to implicitly use all around the tokens, although obviously I did not. I'll have to think about whether I want to change the behavior. I think doing so would be more correct as this aligns with what stopifnot does.

Thanks for raising this; hopefully in the meantime the workaround is painless enough. It will probably be a while before I get back to updating this package.

brodieG commented 6 years ago

Aside, the following will be much faster:

vet(integer() && all_bw(., 10000000L, 99999999L), ...)
franknarf1 commented 6 years ago

Thanks. Yeah, I've applied my zero-length workaround already (only needed in in nine places so far); painless enough for now. I actually wrote all of my tokens like all(cond) at first since I misunderstood and thought that was idiomatic, later removing the alls for readability thinking they weren't needed. Oops.

Thanks for the pointer to all_bw as well. I was still on 0.1.0 and didn't realize there were new releases out... I should probably pay more attention to that sort of thing before filing issues.

brodieG commented 6 years ago

@franknarf1 fyi, I am planning on changing the behavior where 0 length logicals are treated as true as in all(logical()).