Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

refactor map macro for more general use#3850

Merged
gui1117 merged 4 commits intoparitytech:masterfrom
yjhmelody:refactor-map-macro
Oct 22, 2019
Merged

refactor map macro for more general use#3850
gui1117 merged 4 commits intoparitytech:masterfrom
yjhmelody:refactor-map-macro

Conversation

@yjhmelody
Copy link
Contributor

@yjhmelody yjhmelody commented Oct 18, 2019

Signed-off-by: yjhmelody 465402634@qq.com

Signed-off-by: yjhmelody <465402634@qq.com>
@parity-cla-bot
Copy link

It looks like @yjhmelody signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

#[macro_export]
macro_rules! map {
($( $name:expr => $value:expr ),*) => (
($( $name:expr => $value:expr ),* ,) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

can use $(,)? to handle optional trailing comma

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I forget the syntax.

Signed-off-by: yjhmelody <465402634@qq.com>
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

substrate is using tabulation for indent

@kianenigma kianenigma added the A0-please_review Pull request needs code review. label Oct 18, 2019
@bkchr
Copy link
Member

bkchr commented Oct 18, 2019

Can you please explain the reasoning behind your changes? I don't see how that generalizes the macro.

@gui1117
Copy link
Contributor

gui1117 commented Oct 18, 2019

Can you please explain the reasoning behind your changes? I don't see how that generalizes the macro.

That just allow to declare with a trailing comma

@bkchr
Copy link
Member

bkchr commented Oct 18, 2019

Can you please explain the reasoning behind your changes? I don't see how that generalizes the macro.

That just allow to declare with a trailing comma

The trailing comma was added after there was a suggestion in this pr. 🤷‍♀️

@gui1117
Copy link
Contributor

gui1117 commented Oct 18, 2019

Can you please explain the reasoning behind your changes? I don't see how that generalizes the macro.

That just allow to declare with a trailing comma

The trailing comma was added after there was a suggestion in this pr. woman_shrugging

no before the fix trailing comma was made mandatory

@bkchr
Copy link
Member

bkchr commented Oct 18, 2019

I checked now the first commit and understand it. I just saw in the diff the the optional trailing comma was added.

@bkchr
Copy link
Member

bkchr commented Oct 22, 2019

@yjhmelody status?

yjhmelody and others added 2 commits October 22, 2019 16:17
Co-Authored-By: thiolliere <gui.thiolliere@gmail.com>
Co-Authored-By: thiolliere <gui.thiolliere@gmail.com>
@gui1117 gui1117 merged commit 2e1d831 into paritytech:master Oct 22, 2019
@yjhmelody yjhmelody deleted the refactor-map-macro branch October 22, 2019 09:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants