Add assertions on input types #62
Conversation
R/elasticsearch_parsers.R
Outdated
| , assertthat::is.number(max_hits) | ||
| , max_hits >= 0 | ||
| , assertthat::is.number(n_cores) | ||
| , n_cores >= 1 |
There was a problem hiding this comment.
Could you change the n_cores assertions to assertthat::is.count(n_cores)? is.count checks to make sure it's a single positive integer.
Unrelatedly, I wish is.count had a flag or something to allow 0, because checking for a single nonnegative integer is pretty tricky right now.
There was a problem hiding this comment.
yeah good call. I actually don't mind that they force you to break it into two things. More explicit is better than controlling with flags!
There was a problem hiding this comment.
Definitely true that more explicit is better, but in my intuition 0 should technically be a valid "count" haha. So maybe that's the root of my complaint.
R/elasticsearch_parsers.R
Outdated
| , assertthat::is.number(n_cores) | ||
| , n_cores >= 1 | ||
| , assertthat::is.flag(break_on_duplicates) | ||
| , assertthat::is.flag(ignore_scroll_restriction) |
There was a problem hiding this comment.
May want to check that these args aren't NA. Unfortunately assertthat::is.flag(NA) is TRUE because NAs are logical by default.
There was a problem hiding this comment.
ah yep, good catch
R/elasticsearch_eda_funs.R
Outdated
| , assertthat::is.string(use_na) | ||
| , use_na != "" | ||
| , assertthat::is.number(max_terms) | ||
| , max_terms > 0 |
There was a problem hiding this comment.
is.count would work here too.
jameslamb
force-pushed
the
added_assertions
branch
from
2148879 to
aa28c1d
Compare
|
@austin3dickey I just made the changes you recommended. The build failing was just Should hopefully build 🤞 |
jameslamb
force-pushed
the
added_assertions
branch
2 times, most recently
from
97e6199 to
4ae0726
Compare
Codecov Report
@@ Coverage Diff @@
## master #62 +/- ##
=========================================
- Coverage 69.52% 68.43% -1.1%
=========================================
Files 3 4 +1
Lines 502 548 +46
=========================================
+ Hits 349 375 +26
- Misses 153 173 +20
Continue to review full report at Codecov.
|
|
@austin3dickey this is building now and I made the recommended changes, so gonna merge it up. This will look (in a Github email) like I "dismissed your review", but I actually did do all the things. Kind of wish this had tasks like Bitbucket |
jameslamb
dismissed
austin3dickey’s stale review
I took care of these changes, "dismissing" the review so I can merge
|
@austin3dickey ha joke's on me apparently I need one positive review no matter what to merge. At your earliest convenience, no rush |
R/elasticsearch_eda_funs.R
Outdated
| , assertthat::is.string(use_na) | ||
| , use_na != "" | ||
| , assertthat::is.count(max_terms) | ||
| , max_terms > 0 |
There was a problem hiding this comment.
This is redundant - is.count ensures it's 1 or greater
There was a problem hiding this comment.
whoops, yep will fix
R/elasticsearch_parsers.R
Outdated
| , assertthat::is.string(scroll) | ||
| , scroll != "" | ||
| , assertthat::is.count(max_hits) | ||
| , max_hits >= 0 |
There was a problem hiding this comment.
Yeah this is the problem I was talking about earlier. This won't allow max_hits to be 0 because is.count ensures it's greater than 1 :( personally I think that's unintuitive
There was a problem hiding this comment.
blegh oh right, now I understand. Ok I'm going to remove this one. max_hits = 0 is a plausible value.
There was a problem hiding this comment.
also I agree that it's unintuitive
jameslamb
force-pushed
the
added_assertions
branch
from
4ae0726 to
9354560
Compare
|
ugh I think I know what this is. Trying locally, I was seeing a bunch of errors about multiple types not being allowed. The shakespeare data has mixed types :/
|
jameslamb
force-pushed
the
added_assertions
branch
from
9354560 to
e7c66f5
Compare
|
This is working now! Gonna ship it |
This PR introduces
assertthatas a dependency and adds a bunch of assertions on inputs. This will help in debugging and add extra protection against unintended queries being executed on users' ES clusters.