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

rustdoc: Remove Derived Implementations title #34105

Merged
merged 1 commit into from
Jul 1, 2016

Conversation

ollie27
Copy link
Member

@ollie27 ollie27 commented Jun 5, 2016

As far as I know whether a trait was derived or not does not change the
public API so there is no need to include this information in the docs.

This title currently just adds an extra divide in the list of trait
implementations which I don't think needs to be there.

As far as I know whether a trait was derived or not does not change the
public API so there is no need to include this information in the docs.

This title currently just adds an extra divide in the list of trait
implementations which I don't think needs to be there.
@rust-highfive
Copy link
Collaborator

r? @cmr

(rust_highfive has picked a reviewer for you, use r? to override)

@durka
Copy link
Contributor

durka commented Jun 6, 2016

Hmm I don't agree, because when an impl is derived you know exactly what it does, as opposed to a manual impl which may be customized.

@alexcrichton
Copy link
Member

Initially added in #12961 there's a little bit of discussion there, but not a lot. I think historically the impl section was much more noisy than it is today which was the original motivation for that, so the initial rationale may no longer be applicable.

I agree with @durka that this does serve a nice indicator that the implementation wasn't touched and you may be able to rely on that, but on the other hand @ollie27 is right in that it's just visual clutter because that's not really applicable for knowing the API of a type. Whether or not it was strictly implemented via #[derive] should be an implementation detail that's allowed to change over time (and likely will).

As a result I'd personally be in favor of this, but would be curious what @steveklabnik and other docs folks think as well.

@alexcrichton alexcrichton assigned alexcrichton and unassigned emberian Jun 6, 2016
@GuillaumeGomez
Copy link
Member

I'd like to have a live version to see the change directly. Could you add it please?

@ollie27
Copy link
Member Author

ollie27 commented Jun 7, 2016

Sure, why not, there you go: https://ollie27.github.io/rust_doc_test/

Some nice examples:
TypeId: before after
Ipv4Addr: before after

@steveklabnik
Copy link
Member

Whether or not it was strictly implemented via #[derive] should be an implementation detail that's allowed to change over time (and likely will).

This is the way that I feel about it. This feels like a violation of encapsulation to me, but I also don't care enough to block it; if others feel it's useful, I'm okay with it landing.

@GuillaumeGomez
Copy link
Member

I like this change! 👍

@alexcrichton
Copy link
Member

@steveklabnik note that this is removing the difference between sections, whereas currently there is a special "derived implementations" section. Sounds like that's what you'd favor though?

@alexcrichton
Copy link
Member

@bors: r+

Ah looks like this fell off the radar, but seems like there's general favor, thanks @ollie27!

@bors
Copy link
Contributor

bors commented Jun 28, 2016

📌 Commit cc18104 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jun 29, 2016

⌛ Testing commit cc18104 with merge 03ab565...

jseyfried added a commit to jseyfried/rust that referenced this pull request Jun 29, 2016
…hton

rustdoc: Remove Derived Implementations title

As far as I know whether a trait was derived or not does not change the
public API so there is no need to include this information in the docs.

This title currently just adds an extra divide in the list of trait
implementations which I don't think needs to be there.
@bors
Copy link
Contributor

bors commented Jun 29, 2016

⛄ The build was interrupted to prioritize another pull request.

@bors
Copy link
Contributor

bors commented Jun 29, 2016

⌛ Testing commit cc18104 with merge 47695a1...

@bors
Copy link
Contributor

bors commented Jun 29, 2016

⛄ The build was interrupted to prioritize another pull request.

@bors
Copy link
Contributor

bors commented Jun 29, 2016

⌛ Testing commit cc18104 with merge bae0709...

@bors
Copy link
Contributor

bors commented Jun 29, 2016

⛄ The build was interrupted to prioritize another pull request.

@bors
Copy link
Contributor

bors commented Jun 29, 2016

⌛ Testing commit cc18104 with merge d6de998...

@bors
Copy link
Contributor

bors commented Jun 29, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@GuillaumeGomez
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Jun 29, 2016

💔 Test failed - auto-linux-64-x-android-t

@GuillaumeGomez
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Jun 29, 2016

💔 Test failed - auto-linux-64-x-android-t

@alexcrichton
Copy link
Member

@bors: retry

On Wed, Jun 29, 2016 at 9:09 AM, bors [email protected] wrote:

💔 Test failed - auto-linux-64-x-android-t
http://buildbot.rust-lang.org/builders/auto-linux-64-x-android-t/builds/9596


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#34105 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAD95HV9Cul4znBaFAFqysg_MsRiS-vnks5qQpifgaJpZM4Iud-f
.

@alexcrichton
Copy link
Member

@bors: retry

bors added a commit that referenced this pull request Jun 30, 2016
Rollup of 5 pull requests

 - Successful merges: #34105, #34305, #34512, ~~#34531,~~ #34547
@bors
Copy link
Contributor

bors commented Jun 30, 2016

⌛ Testing commit cc18104 with merge 319138d...

@bors
Copy link
Contributor

bors commented Jun 30, 2016

💔 Test failed - auto-linux-64-opt-rustbuild

@alexcrichton
Copy link
Member

@bors bors merged commit cc18104 into rust-lang:master Jul 1, 2016
@ollie27 ollie27 deleted the rustdoc_derived branch July 1, 2016 18:44
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.

8 participants