Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Custom attribute for binary serialization #3922

Merged
merged 7 commits into from
Dec 21, 2016
Merged

Custom attribute for binary serialization #3922

merged 7 commits into from
Dec 21, 2016

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Dec 20, 2016

because custom derives are going to become obsolete

Also makes binary serialization optional on feature

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Dec 20, 2016
@@ -17,7 +17,7 @@
extern crate ethcore_ipc_codegen;

fn main() {
ethcore_ipc_codegen::derive_binary("src/types/mod.rs.in").unwrap();
ethcore_ipc_codegen::derive_binary_cond("src/types/mod.rs.in", cfg!(feature="ipc")).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't 100% understand the use of adding the flag parameter here. having a function take a bool parameter saying whether it should run is pretty strange. I think it's also already covered with the cfg_attr guards that have been added everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah forgot to remove it, originally it was just stripping attributes

@rphmeier rphmeier added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 20, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 29c1fee on binary-opt into ** on master**.

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Dec 20, 2016
@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 20, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 17ed223 on binary-opt into ** on master**.

@gavofyork gavofyork merged commit af501e6 into master Dec 21, 2016
@arkpar arkpar deleted the binary-opt branch January 10, 2017 11:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants