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): Avoid name collisions with users #2935

Merged
merged 1 commit into from
Oct 23, 2021
Merged

Conversation

epage
Copy link
Member

@epage epage commented Oct 23, 2021

PR #2751 highlighted a problem we have where the variable names we use
could collide with users. Rather than parse out when or not to use
special names, and worry about people keeping that up to date through
refactors, I globally renamed all variables by adding a __clap_
prefix, which looks like what serde does to solve this problem.

I audited the result with cargo expand. I didn't add any tests
because any tests would be reactionary and would give us a false sense
of protection since any new code could hit this with anything we do.
Our best route for naming is consistency so people are likely to notice
and copy.

Fixes #2934

PR clap-rs#2751 highlighted a problem we have where the variable names we use
could collide with users.  Rather than parse out when or not to use
special names, and worry about people keeping that up to date through
refactors, I globally renamed all variables by adding a `__clap_`
prefix, which looks like what serde does to solve this problem.

I audited the result with `cargo expand`.  I didn't add any tests
because any tests would be reactionary and would give us a false sense
of protection since any new code could hit this with anything we do.
Our best route for naming is consistency so people are likely to notice
and copy.

Fixes clap-rs#2934
@epage
Copy link
Member Author

epage commented Oct 23, 2021

Because this is collision heavy, I'm going ahead and merging this. If there is iteration to be done, we can do it in another PR.

bors r+

@epage epage added the M-unreviewed Meta: Request for post-merge review. label Oct 23, 2021
@bors
Copy link
Contributor

bors bot commented Oct 23, 2021

Build succeeded:

@bors bors bot merged commit 3f933e1 into clap-rs:master Oct 23, 2021
@epage epage deleted the conflict branch October 23, 2021 21:03
@pksunkara pksunkara removed the M-unreviewed Meta: Request for post-merge review. label Oct 24, 2021
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.

enum with variant field name: T unexpectedly imposes &mut T: PartialEq<&str> in 3.0.0-beta.5
2 participants