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

Implemented (de)serialization for nonzero integer #66

Merged
merged 4 commits into from
Jan 18, 2022

Conversation

real-felix
Copy link

No description provided.

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

Mutually exclusive with #35. This one is more concise and more tests, but also was opened later

@real-felix
Copy link
Author

Do you know when it's reviewed? I need this change, and there're very few line to review.

@matklad matklad enabled auto-merge (squash) January 18, 2022 11:52
@matklad
Copy link
Contributor

matklad commented Jan 18, 2022

Yeah, let's merge this, the impl and tests look good to me!

@matklad
Copy link
Contributor

matklad commented Jan 18, 2022

@real-felix
Copy link
Author

real-felix commented Jan 18, 2022

facepalm travis refuses to go away, opened https://near.zulipchat.com/#narrow/stream/308449-nearinc.2Fcore-tools/topic/borsh.20repo.20admin

I hope it'll be solved soon 😛
I think that the error message could be better, because it's a bit cryptic for now. What do you think about "Zero value read for a NonZero integer" or something like that? (I'm not a native English speaker, I'm not sure about how bad this sounds).

@matklad
Copy link
Contributor

matklad commented Jan 18, 2022

or something like that?

No strong feelings one way or another. My heuristic would be to check what serde_json does and copy that without much extra delibiration.

@real-felix
Copy link
Author

My heuristic would be to check what serde_json does and copy that without much extra delibiration.

Did that, thanks! I think it's important that people understand easily what happens when reading an error message.

@frol frol merged commit 9152e0f into near:master Jan 18, 2022
@real-felix
Copy link
Author

@frol Thanks for the merge. Is it possible to publish this change (maybe as a patch version?)

@frol
Copy link
Collaborator

frol commented Jan 23, 2022

@miraclx Are you actively working on getting the automated publishing pipeline for borsh-rs or should I publish it manually for now?

@miraclx
Copy link
Contributor

miraclx commented Jan 24, 2022

It's complete, pending review: #67

@miraclx
Copy link
Contributor

miraclx commented Jan 26, 2022

Released on https://github.com/near/borsh-rs/releases/tag/v0.9.2

Crate Links

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.

5 participants