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

wip: Clean up lifetimes, whitespace at end of lines and folded multi line function definitions into one where suitable #249

Closed
wants to merge 1 commit into from

Conversation

WildCryptoFox
Copy link

Quick in vim, cleaned up lifetimes for App Arg and SubCommand. Somewhere I broke something. 😉

DO NOT MERGE


That's me for the night. 💤

…line function definitions into one where suitable
@WildCryptoFox WildCryptoFox added C-bug Category: Updating dependencies R: work in progress labels Sep 10, 2015
@WildCryptoFox WildCryptoFox self-assigned this Sep 10, 2015
@kbknapp kbknapp added the M-breaking-change Meta: Implementing or merging this will introduce a breaking change. label Sep 10, 2015
@kbknapp
Copy link
Member

kbknapp commented Sep 10, 2015

We'll need to a scrub (as best we can) to see how many code bases this affects. Since this is a breaking change for anyone returning an Arg or App from a function call.

Don't get me wrong, I'm ok with a breaking change for a huge ergonomic win, I just want to be as political about it as possible so as not to make too many people angry that APIs are changing unstably underneath them.

The harder to assess breaking change of this PR is when someone is using a 'static lifetime for any of the "settings" builder methods, and a non 'static lifetime for any of the others because Rust will complain that the non-static lifetime doesn't live long enough. I haven't checked this PR too deeply, but a potential way to solve this is saying all those "settings" lifetimes last at least as long as the Arg name (which must persist as long as the returned ArgMatches is alive.

Thoughts, comments?

@WildCryptoFox WildCryptoFox mentioned this pull request Sep 11, 2015
2 tasks
@WildCryptoFox
Copy link
Author

The lifetime for 'a as applied to it's fields already guarantees this. It is not saying that they must all have the same lifetime.

And yes, this would be a breaking change and can wait for the next major as it is a low priority.

@kbknapp
Copy link
Member

kbknapp commented Sep 17, 2015

Sounds good. The major feature I'd be willing to bump to a 2.x is automatic serialization into a Rust struct (a la docopt), so 2.x may be here sooner rather than later.

@WildCryptoFox
Copy link
Author

@kbknapp Serialization as in #146? That issue is heavily neglected for discussion

@WildCryptoFox
Copy link
Author

Closing in favor of #258. I'll include the code cleanup after CowStr.

epage added a commit that referenced this pull request Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants