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

Byte decode error #89

Closed
wants to merge 3 commits into from

Conversation

MarinPostma
Copy link
Contributor

@MarinPostma MarinPostma commented Dec 31, 2020

close #88

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I tried but haven't finished yet. So, you take the lead and I will accept your PR!


I am wondering if introducing a custom public Result type named CodecResult, could be better.

pub type CodecResult<T> = Result<T, Box<dyn Error>>;

}

None
Err("The provided bytes do not satisfy the alignment requirements.")?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I switch from zerocopy to bytemuck I will change this error string into a nicer error, more precise!

@@ -71,8 +71,8 @@ pub struct DecodeIgnore;
impl heed_traits::BytesDecode<'_> for DecodeIgnore {
type DItem = ();

fn bytes_decode(_bytes: &[u8]) -> Option<Self::DItem> {
Some(())
fn bytes_decode(_bytes: &[u8]) -> Result<Self::DItem, Box<dyn std::error::Error>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn bytes_decode(_bytes: &[u8]) -> Result<Self::DItem, Box<dyn std::error::Error>> {
fn bytes_decode(_bytes: &[u8]) -> Result<Self::DItem, Box<dyn Error>> {

By importing the Error trait into the scope.

@@ -278,7 +278,7 @@ impl PolyDatabase {
{
assert_eq!(self.env_ident, txn.env.env_mut_ptr() as usize);

let key_bytes: Cow<[u8]> = KC::bytes_encode(&key).ok_or(Error::Encoding)?;
let key_bytes: Cow<[u8]> = KC::bytes_encode(&key).map_err(|e| Error::Encoding(e))?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let key_bytes: Cow<[u8]> = KC::bytes_encode(&key).map_err(|e| Error::Encoding(e))?;
let key_bytes: Cow<[u8]> = KC::bytes_encode(&key).map_err(Error::Encoding)?;

Comment on lines +83 to +84
Encoding(Box<dyn std::error::Error>),
Decoding(Box<dyn std::error::Error>),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Encoding(Box<dyn std::error::Error>),
Decoding(Box<dyn std::error::Error>),
Encoding(Box<dyn Error>),
Decoding(Box<dyn Error>),

@MarinPostma
Copy link
Contributor Author

I am wondering if introducing a custom public Result type named CodecResult, could be better.

Why not exposing an anyhow Result instead?

@Kerollmops
Copy link
Member

Kerollmops commented Jan 3, 2021

Why not exposing an anyhow Result instead?

I don't want to bring one more dependency in this crate, don't want to force users to use anyhow, using anyhow here would not bring interesting features here, and anyhow has been design to be used by binary crates.

Exposing a type alias will work just fine :)

@Kerollmops Kerollmops mentioned this pull request Jan 16, 2021
@Kerollmops Kerollmops added the breaking A change that is breaking the semver label Jan 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A change that is breaking the semver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support custom encoding/decoding errors
2 participants