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(derive): Add "id" attribute #4049

Merged
merged 1 commit into from
Aug 9, 2022
Merged

fix(derive): Add "id" attribute #4049

merged 1 commit into from
Aug 9, 2022

Conversation

danielparks
Copy link
Contributor

Previously the Arg id was set with the "name" attribute. This allows use of an "id" attribute to match the underlying struct.

A side effect of this is that the "id" attribute may also be used on Commands. This isn't desired, but given the current architecture of the attribute parser, it's hard to avoid.

Fixes: #3785

src/_derive/mod.rs Outdated Show resolved Hide resolved
@danielparks
Copy link
Contributor Author

I think the lint check failure is unrelated to my changes. The commit subject is slightly longer than 50 characters, though. I can fix that if you like.

@epage
Copy link
Member

epage commented Aug 9, 2022

I think the lint check failure is unrelated to my changes. The commit subject is slightly longer than 50 characters, though. I can fix that if you like.

The lint failure is

96c57b0: error Subject should be capitalized but found allow

I need to go back and do some fixes to committed since its still not all working with the git security fix sigh

@epage
Copy link
Member

epage commented Aug 9, 2022

Note: it also says to not fuss too much over the commit messages. Its preferred but not required to have them be clean but there isn't a good way to do that with github actions

Previously the Arg id was set with the "name" attribute. This allows use
of an "id" attribute to match the underlying struct.

A side effect of this is that the "id" attribute may also be used on
Commands. This isn't desired, but given the current architecture of the
attribute parser, it's hard to avoid.

Fixes: #3785
@danielparks
Copy link
Contributor Author

Thanks. It was easy to fix, so I went ahead.

@danielparks danielparks changed the title fix(derive): allow setting Arg id using "id" attribute fix(derive): Add "id" attribute Aug 9, 2022
@epage epage merged commit a98bcb4 into clap-rs:master Aug 9, 2022
@danielparks danielparks deleted the issue-3785-derive-id-attr branch August 9, 2022 19:33
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.

Derive API doesn't respect id attribute
2 participants