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

fix: signed packet from bytes #57

Merged
merged 1 commit into from
Mar 1, 2024
Merged

Conversation

Frando
Copy link
Contributor

@Frando Frando commented Mar 1, 2024

#56 had a mistake in the from_bytes method... Here's the fix, this time with the test to prevent mistakes like this again.

@Nuhvi
Copy link
Collaborator

Nuhvi commented Mar 1, 2024

@Frando Irrelevant but do you think the naming of the methods need changing to be idiomatic? like try_from_bytes instead? Similarly, is the as_ methods naming appropriate because cloning Bytes is cheap?

Sorry for the newbie questions, just want to leverage the V2 to most :)

@Nuhvi Nuhvi merged commit 51ee224 into pubky:main Mar 1, 2024
1 check passed
@Frando
Copy link
Contributor Author

Frando commented Mar 1, 2024

Yeah I am a bit unsure there usually myself... The general convention is:
as_ -> gives you a reference to some inner data, always cheap
to_ -> gives you an owned copy of some inner data, always cheap
into_ -> gives you an owned value, can be expensive

So it should be to_bytes and not as_bytes because you get an owned Bytes and not a reference.

not sure when it's idiomatic to do try_ prefixes to be honest. I think in std it's try_from to not conflict with from trait, and for regular methods it's try_into_foo if there's also a into_foo which would panic in case of error. E.g currently a lot of try_with_capacity etc methods are being added to Vec etc to fail with error and not panic if allocation fails.

@Nuhvi
Copy link
Collaborator

Nuhvi commented Mar 1, 2024

Hmm I thought try_ should be anything that returns Result. Will read more and update it in v2.

@Frando
Copy link
Contributor Author

Frando commented Mar 1, 2024

Yea I've seen different things in different codebases. E.g. in databases or so where each operation involves IO it's not common to add try_ prefixes because all methods would have try_. For fallible conversions I've seen both. Maybe there try_ is more idiomatic, not sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants