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

Add common traits to public types #472

Closed
wants to merge 2 commits into from

Conversation

@sterlingjensen
Copy link
Contributor

sterlingjensen commented Nov 28, 2018

Improves interoperability, making downstream manual implementation unnecessary.
rust-lang-nursery/api-guidelines
C-COMMON-TRAITS


This change is Reviewable

Improves interoperability, making downstream manual implementation unnecessary.
rust-lang-nursery/api-guidelines
C-COMMON-TRAITS
Deriving PartialOrd and Ord results in "error[E0389]: cannot borrow data mutably in a `&` reference"
@SimonSapin
Copy link
Member

SimonSapin commented Nov 28, 2018

Thank you for this pull request. I’d prefer individual changes to be motivated by more than just guidelines.

A lot of of these impls seem to be of very dubious use. For example the PathSegmentsMut type only exists to facilitate a method-chaining API style. It is not a "data" type that users of the crate are expected to use in their own types. It feels that the approach to this PR was to add as many derives as possible wherever the compiler would allow them. Guidelines are not law, they should be evaluated on a case-by-case basis.

As the name suggests, HostInternal is not part of the public API. So implementing more traits for it does not help anyone and only adds to compile times. The same applies to some other types too (though their name might not make it as obvious).

Deriving Default for Url is wrong. It crates non-sense Url values that violate internal invariants of the type. I haven’t checked if other impls are similarly wrong.

Some other impls, while not exactly wrong, don’t seem to make much sense. For example OpaqueOrigin is intended to be opaque. It is by design that it doesn’t provide (much) more behavior than equality comparison. An arbitrary ordering between opaque origins is not meaningful.

Code style changes (in this case re-ordering derive names) in the same commit as other changes makes it hard to see what the significant changes are (in this case which derives were added).

When mentioning an external document that is on the web, providing an URL would be nice. These guidelines are on multiple separate pages, so there is no easy way to CTRL-F through them to find https://rust-lang-nursery.github.io/api-guidelines/interoperability.html#types-eagerly-implement-common-traits-c-common-traits

@sterlingjensen
Copy link
Contributor Author

sterlingjensen commented Nov 29, 2018

The changes are motivated by my experience in integrating the lib and having to write a bunch of tests that ensure hashing isn't broken. The guidelines are a rationalization after the fact. I can appreciate the perspective that a lot of the traits don't make sense within the lib, but internal utility isn't the point of publicly exported traits. For example, the Default on Url: I have a struct, returned only after successfully processing, that includes the Url struct - it is nonsensical in that case to use Option<Url>, implement fmt::Display in order to handle that Option instead of just relying on the existing impl fmt::Display for Url... etc. HostInternal is a very nice field to sort a collection of Urls on when you're application benefits from a consideration to the authority portion of the URI.

Long story short: maintaining the ability to sort by error type after integrating ParseError could have been easier.

@SimonSapin
Copy link
Member

SimonSapin commented Nov 29, 2018

I’m having trouble understanding how the different parts of your last message relate to each other. While some of the above is debatable,

  • Url: Default is definitely incorrect and I will not merge it. It returns an Url value where most methods panic because of broken internal state.
  • HostInternal is not part of the public API. It is impossible to use outside of the url crate.
@sterlingjensen
Copy link
Contributor Author

sterlingjensen commented Nov 29, 2018

You're absolutely right, I now see that sorting on the host function only touches HostInternal for the match. The only reason to expand traits for it would be for Url, but you've made your view on that clear, so both of those can be scratched off.

@sterlingjensen sterlingjensen deleted the sterlingjensen:traitinterop branch Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.