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

Implement and Derive common traits #952

Closed
52 tasks
Tracked by #950
kbknapp opened this issue May 9, 2017 · 14 comments · Fixed by #2266
Closed
52 tasks
Tracked by #950

Implement and Derive common traits #952

kbknapp opened this issue May 9, 2017 · 14 comments · Fixed by #2266
Labels
C-enhancement Category: Raise on the bar on expectations E-easy Call for participation: Experience needed to fix: Easy / not much E-medium Call for participation: Experience needed to fix: Medium / intermediate M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Milestone

Comments

@kbknapp
Copy link
Member

kbknapp commented May 9, 2017

Eagerly derive/implement common traits for all public types. I haven't yet looked at which ones would actually be appropriate to implement. The list below is just a quick list.

I'm not 100% sure, but I think adding these would be a breaking change?

  • App
    • Clone
    • Eq
    • PartialEq
    • Ord
    • PartialOrd
    • Hash
    • Debug,
    • Display
    • Default
  • Arg
    • Clone
    • Eq
    • PartialEq
    • Ord
    • PartialOrd
    • Hash
    • Debug,
    • Display
    • Default
  • ArgMatches
    • Clone
    • Eq
    • PartialEq
    • Ord
    • PartialOrd
    • Hash
    • Debug,
    • Display
    • Default
  • Values
    • Copy
    • Clone
    • Eq
    • PartialEq
    • Ord
    • PartialOrd
    • Hash
    • Debug,
    • Display
    • Default
  • ValuesOs
    • Copy
    • Clone
    • Eq
    • PartialEq
    • Ord
    • PartialOrd
    • Hash
    • Debug,
    • Display
    • Default
@kbknapp kbknapp added D: easy M-breaking-change Meta: Implementing or merging this will introduce a breaking change. E-medium Call for participation: Experience needed to fix: Medium / intermediate C-enhancement Category: Raise on the bar on expectations labels May 9, 2017
@kbknapp kbknapp added this to the Lib Blitz Overhaul milestone May 9, 2017
@Vinatorul
Copy link
Contributor

I can work on this. I am not sure about breaking change right now, it depends from associated changes.

@kbknapp
Copy link
Member Author

kbknapp commented Jun 18, 2017

Yeah I'm concerned with the breaking changes implications. I've got a local 3x branch that I've been working on the past few days. Once I get it to a state where it actually builds I'll push the branch and you can run through the types making sure they're all deriving the traits they should. 👍

I've got one more work trip coming up soon, but I'm hoping to have this branch pushed sometime this coming week.

@Vinatorul
Copy link
Contributor

Nice, is there any issues I can work on while you will be away? Are you still using gitter?

@kbknapp
Copy link
Member Author

kbknapp commented Jun 20, 2017

@Vinatorul The gitter is still up, and I get pinged on mobile (usually) when new messages are posted. Really any of the current feature requests could get knocked out, but 3.x is being heavily refactored, so it'll probably just be easier to wait until I get it building.

I now have a 3x-dev branch up, but it doesn't compile yet as it's a messy state of partially refactored. I'm hoping to get a large chunk of it complete tomorrow while on an airplane 😄 Once the refactoring is done, adding the new features will be easy, and you can jump on in at that point. I'll publish a list of priorities as well.

Once it's good to go, I'll create a 3x-master branch and start merging feature PRs onto that branch.

@kbknapp kbknapp modified the milestones: Lib Blitz Overhaul, v3-alpha1 Feb 2, 2018
@kbknapp kbknapp added the E-easy Call for participation: Experience needed to fix: Easy / not much label Jul 22, 2018
@pksunkara pksunkara modified the milestones: v3-alpha.2, v3.0 Feb 1, 2020
@sassman
Copy link

sassman commented May 6, 2020

Can this issue be tackled now? If that is the case, I'd might have a look at it.

@pksunkara
Copy link
Member

Yes, please. Which is why I added it to the rust weekly.

@sassman
Copy link

sassman commented May 6, 2020

There I saw it and it might be an easy candidate.

@CreepySkeleton
Copy link
Contributor

CreepySkeleton commented May 6, 2020

Regarding certain traits:

  • Default is meaningless for Arg, App, ArgGroup.
  • Implementing PartialOrd + Ord is probably next to impossible because they aren't implemented for IndexMap. Same with Hash

@kbknapp
Copy link
Member Author

kbknapp commented May 6, 2020

With #1365 we should look at how much this increases the binary size and probably feature gate this (it's ok if it's in the default features) if it negatively effects the size. I think LLVM is pretty good about dead code elimination and removing any trait impls that we impl (or derive) but aren't actually used in the resulting binary...but I'm not 100% certain in all cases. I know there are specific cases where LLVM won't remove dead code, but I believe that is tied to generics and monomorphic trait impls IIRC.

@pickfire
Copy link
Contributor

@sassman are you working on all of it or do you need some help?

@sassman
Copy link

sassman commented May 13, 2020

@pickfire I started by PartialEq on ArgGroup and App and wrote some little tests for it. But I'm not sure if tests would be necessary for those standard traits.

@pksunkara
Copy link
Member

Tests are not needed. Just write derive(PartialEq) and if it compiles, then it's okay. That is why this is a beginner issue 😄

@pksunkara
Copy link
Member

Tests are only needed if there is a custom implementation somewhere.

@pickfire
Copy link
Contributor

@pksunkara It would be fun if beginners are able to claim parts of this issue like rust-dc/fish-manpage-completions#2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Raise on the bar on expectations E-easy Call for participation: Experience needed to fix: Easy / not much E-medium Call for participation: Experience needed to fix: Medium / intermediate M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants