-
Notifications
You must be signed in to change notification settings - Fork 1
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
Upkeep #27
Conversation
needs to be checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great - that is a really good PR! I just have one question.
@@ -107,7 +107,7 @@ test_that("check_flob_query", { | |||
expect_identical(check_flob_query(slob_obj, slob = TRUE), slob_obj) | |||
expect_identical(check_flob_query(slob_obj, slob = NA), slob_obj) | |||
|
|||
expect_error(check_flob_query("non-blob", slob = NA), "`x` must be a blob of a serialized object.") | |||
expect_error(check_flob_query("non-blob", slob = TRUE), "`x` must be a blob of a serialized object.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if slob = NA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error:
! `check_flob_query("non-blob", slob = NA)` threw an error with unexpected message.
Expected match: "`x` must be a blob of a serialized object."
Actual message: "`x` must be a list of raw vectors"
@joethorley, @sebdalgarno, and @evanamiesgalonski, when you have a chance would you be able to review this pull request? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good - there is a conflict that presumably we can resolve when merge?
The conflict should be easy to resolve when we merge. It's just a difference in the README badges |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Once we've merged I'll do a pass through at some point and clean up some of the unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good thanks Duncan!
I didn't really know who to put as a reviewer for this, so I decided to be safe and go with all three of you as this package is a CRAN package.
I'm going through our packages that use the set functions and making sure the code still works on the development version of R. You can see more info in this issue.
Something that definitely requires your attention is commit e0b9b32.