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

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

Closed
franknarf1 opened this issue Dec 8, 2017 · 4 comments
Closed

Comments

@franknarf1
Copy link
Contributor

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 brodieG changed the title option for vet to treat zero-length logical result as TRUE Should tokens that evaluate to zero-length logicals be treated as TRUE? Dec 8, 2017
@brodieG brodieG added this to the 0.2.3 milestone Dec 8, 2017
@brodieG
Copy link
Owner

brodieG commented Dec 8, 2017

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
Copy link
Owner

brodieG commented Dec 8, 2017

Aside, the following will be much faster:

vet(integer() && all_bw(., 10000000L, 99999999L), ...)

@franknarf1
Copy link
Contributor Author

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
Copy link
Owner

brodieG commented Feb 28, 2018

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

brodieG added a commit that referenced this issue Mar 1, 2018
92: vetr had evaluation env problems
89: zero length vet token results are treated as
    TRUE to conform to stopifnot
@brodieG brodieG closed this as completed in 424e402 Mar 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants