-
Notifications
You must be signed in to change notification settings - Fork 150
RUST-1406 Add crate-wide error types, convert value access errors #551
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
Conversation
3 | ||
] | ||
}"#; | ||
let doc_display_pretty_expectation = "{\n \"hello\": [\n 1, \n 2, \n 3\n ]\n}"; |
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.
Unrelated, but rustfmt was deleting whitespace at the end of lines within the multi-line string literals (which is very bad behavior), so I rewrote them as single-line strings.
src/document.rs
Outdated
pub fn is_null(&self, key: impl AsRef<str>) -> bool { | ||
self.get(key) == Some(&Bson::Null) | ||
/// Returns the unit type if the given key corresponds to a [`Bson::Null`] value. | ||
pub fn get_null(&self, key: impl AsRef<str>) -> Result<()> { |
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.
breaking change here, there's no real value to return but I thought it would be useful to have the same error information as the rest of the getter methods.
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.
Reasonable! Nit: I'd lean towards returning a Bson::Null
in case the user was stuffing the return value back into a Bson
enum or something like that.
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.
makes sense - updated
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.
LGTM! Doc cleanup is very nice.
src/document.rs
Outdated
pub fn is_null(&self, key: impl AsRef<str>) -> bool { | ||
self.get(key) == Some(&Bson::Null) | ||
/// Returns the unit type if the given key corresponds to a [`Bson::Null`] value. | ||
pub fn get_null(&self, key: impl AsRef<str>) -> Result<()> { |
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.
Reasonable! Nit: I'd lean towards returning a Bson::Null
in case the user was stuffing the return value back into a Bson
enum or something like that.
#[non_exhaustive] | ||
ValueAccess { | ||
/// The key of the value. | ||
key: String, |
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.
I wonder if it's worth hoisting this to be an optional element of Error
; I suspect key is relevant data for more types of failure than just this. Probably this'll become clear as the conversion continues :)
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.
yes that could definitely be useful - I'll make the change in a future PR if it comes up!
RUST-1406
This PR introduces crate-wide
Error
andResult
types, which are modeled on the equivalent types in the driver, and does the first pass of error conversions for theDocument
andRawDocument
getter methods. I also did a bit of documentation cleanup for theDocument
methods. I will follow up with more PRs to port over the rest of the error types and finalize the design/documentation of the new types.