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

Derive implementations #39

Merged
merged 2 commits into from
Oct 8, 2019
Merged

Derive implementations #39

merged 2 commits into from
Oct 8, 2019

Conversation

oherrala
Copy link
Contributor

@oherrala oherrala commented Oct 1, 2019

This PR contains two things:

  • Derive (Partial)Eq, (Partial)Ord and Hash traits for enums. Deriving these five traits allow using these enums in HashMaps and BTreeMaps.

  • Derive Debug for structs in routing netlink subsystem. These make it much easier to figure out what happens by logging or print debugging.

@jbaublitz
Copy link
Owner

Hi @oherrala, thanks for the contributions! I'm going to have to take a look into trait bounds on struct type parameters again before merging this. I had a fairly active core Rust team member caution me about trait bounds on struct type parameters as opposed to the impls on those structs in a specific scenario so I've avoided them elsewhere in neli. I'm going to do some digging, but as I recall when I did this, it introduced some compilation errors in some of the other modules. It looks like it passes CI here, but I want to dig back into what happened and why he recommended against this last time so that I don't cause myself problems down the line. I've bumped into places where they're necessary in the past, and I'd like to see if it makes sense to impose those restrictions elsewhere in neli or if I should keep it as is.

@jbaublitz
Copy link
Owner

Please see this issue for a discussion of trait bounds of struct type parameters: rust-lang/rust-clippy#1689. For now, I'd prefer to have those removed and I've opened an issue to evaluate whether this should indeed be the case in v0.5.0 (#43). I think I will probably maintain this requirement even in v0.5.0 simply because of the example of how easy it is to introduce breaking changes when defining trait bounds directly on struct type parameters. Once they're removed here, I'll merge this.

Copy link
Owner

@jbaublitz jbaublitz left a comment

Choose a reason for hiding this comment

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

Please remove trait bounds on struct type parameters.

@oherrala
Copy link
Contributor Author

oherrala commented Oct 8, 2019

@jbaublitz Thanks! I force pushed new changes without the trait bounds. There was a reason I needed to add the trait bounds on structs, but now removing those I can't reproduce the issue. Dunno what the reason was, but now locally tests pass without!

@jbaublitz
Copy link
Owner

Looks good to me. Thanks for contributing! I'm merging this now.

@jbaublitz jbaublitz merged commit f9b26b7 into jbaublitz:master Oct 8, 2019
@oherrala oherrala deleted the more-debug branch October 8, 2019 11:47
@oherrala
Copy link
Contributor Author

oherrala commented Oct 8, 2019

Thanks!

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.

2 participants