-
Notifications
You must be signed in to change notification settings - Fork 77
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
chore(schema): added schema to bytes::Bytes #330
base: master
Are you sure you want to change the base?
Conversation
/// Adds schema to `bytes::Bytes`. | ||
/// Conservatively just uses `[u8]` schema, so that any consumer already handles it. | ||
/// while in Rust code we can be more optimal as needed. | ||
#[cfg(feature = "bytes")] |
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.
This feature should be declared in Cargo.toml as well.
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.
it probably already is declared as an implicit one from optional dependency https://github.com/near/borsh-rs/blob/master/borsh/Cargo.toml#L41
Hey, thank you for PR. It looks fine to me, but I would want to request a review from @dj8yfo as he worked way more over it |
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,
but i forgot to mention to add an entry to https://github.com/near/borsh-rs/blob/master/.github/test.sh#L46 for the new test to be run. i checked gh actions logs and didn't find it
btw there are some tools to test features and feature combinations: https://github.com/romnn/cargo-feature-combinations https://github.com/frewsxcv/cargo-all-features https://github.com/taiki-e/cargo-hack https://github.com/ggwpez/zepter as i see they can automated some work rather than doing sh with subset of possible combinations |
As I see |
No description provided.