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] #1917: Added easy_from_str_impl macro for use with AssetValueType #2054

Merged

Conversation

SamHSmith
Copy link
Contributor

Description of the Change

Created a variadic declarative macro that implements FromStr for C like enums.

Issue

#1917

Benefits

Future FromStr implementations for C like enums can be a lot smaller codesize.

Possible Drawbacks

May be harder to read and understand for some.

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Apr 4, 2022
appetrosyan
appetrosyan previously approved these changes Apr 4, 2022
@appetrosyan appetrosyan dismissed their stale review April 4, 2022 11:39

@SamHSmith asked for more time.

@SamHSmith SamHSmith force-pushed the decl_macro_asset_value_type2 branch 2 times, most recently from 84ea691 to 1fdbff9 Compare April 4, 2022 11:49
@SamHSmith SamHSmith force-pushed the decl_macro_asset_value_type2 branch from 1fdbff9 to aeecc31 Compare April 4, 2022 16:23
@SamHSmith SamHSmith force-pushed the decl_macro_asset_value_type2 branch from aeecc31 to 75961f7 Compare April 5, 2022 12:36
}

easy_from_str_impl! {AssetValueType, Quantity, BigQuantity, Fixed, Store}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to mention for future macros. I think that's not a good tradition in Iroha to write such simple to implement but hard to read syntax.

Take a look at this rule:

Rust macros let you dream up practically whatever input syntax you want. Aim to keep input syntax familiar and cohesive with the rest of your users' code by mirroring existing Rust syntax where possible. Pay attention to the choice and placement of keywords and punctuation.

@Arjentix Arjentix merged commit f1f14dc into hyperledger-iroha:iroha2-dev Apr 5, 2022
mversic pushed a commit to mversic/iroha that referenced this pull request May 2, 2022
appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022
appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022
appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022
mversic pushed a commit to mversic/iroha that referenced this pull request May 13, 2022
mversic pushed a commit to mversic/iroha that referenced this pull request May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants