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

Only show methods that appear in impl blocks in the Implementors sections of trait doc pages #61505

Merged
merged 1 commit into from
Jun 19, 2019

Conversation

ebarnard
Copy link
Contributor

@ebarnard ebarnard commented Jun 3, 2019

In the "Implementors" and "Implementations on Foreign Types" sections, only show methods that appear in the impl block for that type. This has the benefit of

  • Reducing the size of the Iterator page, and other large trait documentation pages.
  • Retaining documentation on the impl blocks and functions in the impl blocks.
  • Indicating which provided methods are overridden.
  • Making the documentation match the structure of the code being documented.
  • Being a small change that can be easily backed out if issues arise.

A set of Rust stdlib docs build with this change are available here.

The size of the Iterator doc page is reduced from 14.4MB (latest nightly) to 724kB.

Before:
Screenshot 2019-06-03 at 23 12 17

After:
Screenshot 2019-06-03 at 16 41 27

cc #55900

…ementors and Implementations on Foreign Types sections of trait documentation pages.
@Stargateur
Copy link
Contributor

Stargateur commented Jun 4, 2019

Perfect I think you have found the best solution. Very cleaver. Can't wait to have this in prod.

I wonder if you should do even more and also remove functions in the impl blocks. We only really need the associate type information. If people want more detail then can click on the struct link that implement the trait. Or maybe just show function that have documentation.

So basically, just remove content in "Show hidden undocumented items".

@tesuji
Copy link
Contributor

tesuji commented Jun 4, 2019

cc @GuillaumeGomez

@ebarnard
Copy link
Contributor Author

ebarnard commented Jun 4, 2019

@Stargateur Based on how #55900 has progressed I think there's support for keeping docs on items in impl blocks. Only showing functions with docs could be a bit confusing as it would be the only instance of this behaviour.

"Show hidden undocumented items" does take up a lot of space and could be condensed somehow but I would leave that to a follow up PR.

@Stargateur
Copy link
Contributor

@ebarnard You right about the confusion, so I will simply propose we only keep associate item type and let people click on the struct that implement the trait for full detail of implementation. Just some my through of course.

@GuillaumeGomez
Copy link
Member

I really like this solution like I already said on the issue and the code looks good. We still need other approval from @rust-lang/docs and @rust-lang/rustdoc people.

@tesuji

This comment has been minimized.

@rustbot rustbot added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 5, 2019
@tesuji
Copy link
Contributor

tesuji commented Jun 6, 2019

This is awesome. I had built this PR locally and the rust Iterator doc page size
is really reduced like you said.

@jonas-schievink jonas-schievink added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 10, 2019
@tesuji
Copy link
Contributor

tesuji commented Jun 11, 2019

Ping from triage: Are there any updates here @rust-lang/docs and @rust-lang/rustdoc
members?

I eagerly want to see this land on nightly so people can benefit from this PR.

Hm, I cannot ping those teams: ping @steveklabnik, @rylev, and @ollie27 .

@GuillaumeGomez
Copy link
Member

I'll ping them for you. :)

cc @rust-lang/docs @rust-lang/rustdoc

@GuillaumeGomez
Copy link
Member

Considering how much time has passed, I guess we can just move forward... Thanks!

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 18, 2019

📌 Commit 45bb409 has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2019
@bors
Copy link
Contributor

bors commented Jun 18, 2019

⌛ Testing commit 45bb409 with merge f1f473c13355bc17020a48447ea47fa798fc8003...

@bors
Copy link
Contributor

bors commented Jun 18, 2019

💔 Test failed - checks-travis

@rust-highfive
Copy link
Collaborator

The job i686-apple of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:03:33]       Memory: 8 GB
[00:03:33]       Boot ROM Version: VMW71.00V.7581552.B64.1801142334
[00:03:33]       Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
[00:03:33]       SMC Version (system): 2.8f0
[00:03:33]       Serial Number (system): VMZHZQBV9mVY
[00:03:33] 
[00:03:33] hw.ncpu: 4
[00:03:33] hw.byteorder: 1234
[00:03:33] hw.memsize: 8589934592
---
[00:39:26] [RUSTC-TIMING] quick_error test:false 0.380
[00:39:26]    Compiling rustc_driver v0.0.0 (/Users/travis/build/rust-lang/rust/src/librustc_driver)
[00:39:27]    Compiling crossbeam-utils v0.2.2
[00:39:27]    Compiling log v0.4.6
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 18, 2019
@QuietMisdreavus
Copy link
Member

@bors retry stalled build

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 18, 2019
Only show methods that appear in `impl` blocks in the Implementors sections of trait doc pages

In the "Implementors" and "Implementations on Foreign Types" sections, only show methods that appear in the `impl` block for that type. This has the benefit of
- Reducing the size of the Iterator page, and other large trait documentation pages.
- Retaining documentation on the `impl` blocks and functions in the `impl` blocks.
- Indicating which provided methods are overridden.
- Making the documentation match the structure of the code being documented.
- Being a small change that can be easily backed out if issues arise.

A set of Rust stdlib docs build with this change are [available here](https://ebarnard.github.io/2019-06-03-rust-smaller-trait-implementers-docs/).

The size of the [`Iterator` doc page](https://ebarnard.github.io/2019-06-03-rust-smaller-trait-implementers-docs/std/iter/trait.Iterator.html) is reduced from 14.4MB (latest nightly) to 724kB.

Before:
<img width="1411" alt="Screenshot 2019-06-03 at 23 12 17" src="https://user-images.githubusercontent.com/1059683/58837971-1722a780-8655-11e9-8d81-51e48130951d.png">

After:
<img width="1428" alt="Screenshot 2019-06-03 at 16 41 27" src="https://user-images.githubusercontent.com/1059683/58814907-84ffac80-861e-11e9-8692-79be473a5299.png">

cc rust-lang#55900
Centril added a commit to Centril/rust that referenced this pull request Jun 18, 2019
Only show methods that appear in `impl` blocks in the Implementors sections of trait doc pages

In the "Implementors" and "Implementations on Foreign Types" sections, only show methods that appear in the `impl` block for that type. This has the benefit of
- Reducing the size of the Iterator page, and other large trait documentation pages.
- Retaining documentation on the `impl` blocks and functions in the `impl` blocks.
- Indicating which provided methods are overridden.
- Making the documentation match the structure of the code being documented.
- Being a small change that can be easily backed out if issues arise.

A set of Rust stdlib docs build with this change are [available here](https://ebarnard.github.io/2019-06-03-rust-smaller-trait-implementers-docs/).

The size of the [`Iterator` doc page](https://ebarnard.github.io/2019-06-03-rust-smaller-trait-implementers-docs/std/iter/trait.Iterator.html) is reduced from 14.4MB (latest nightly) to 724kB.

Before:
<img width="1411" alt="Screenshot 2019-06-03 at 23 12 17" src="https://user-images.githubusercontent.com/1059683/58837971-1722a780-8655-11e9-8d81-51e48130951d.png">

After:
<img width="1428" alt="Screenshot 2019-06-03 at 16 41 27" src="https://user-images.githubusercontent.com/1059683/58814907-84ffac80-861e-11e9-8692-79be473a5299.png">

cc rust-lang#55900
bors added a commit that referenced this pull request Jun 19, 2019
Rollup of 11 pull requests

Successful merges:

 - #61505 (Only show methods that appear in `impl` blocks in the Implementors sections of trait doc pages)
 - #61701 (move stray run-pass const tests into const/ folder)
 - #61748 (Tweak transparent enums and unions diagnostic spans)
 - #61802 (Make MaybeUninit #[repr(transparent)])
 - #61839 (ci: Add a script for generating CPU usage graphs)
 - #61842 (Remove unnecessary lift calls)
 - #61843 (Turn down the myriad-closures test)
 - #61896 (rustc_typeck: correctly compute `Substs` for `Res::SelfCtor`.)
 - #61898 (syntax: Factor out common fields from `SyntaxExtension` variants)
 - #61938 (create an issue for miri even in status test-fail)
 - #61941 (Preserve generator and yield source for error messages)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Jun 19, 2019

⌛ Testing commit 45bb409 with merge 274829b9fc8b9ec375adcda95dfa498f20cb95f3...

@Centril
Copy link
Contributor

Centril commented Jun 19, 2019

@bors retry yielding to r0llup

@rust-highfive
Copy link
Collaborator

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
Cloning into 'rust-lang/rust'...
travis_time:end:11e215d4:start=1560916657080239868,finish=1560916662912811726,duration=5832571858
$ cd rust-lang/rust
$ git checkout -qf 274829b9fc8b9ec375adcda95dfa498f20cb95f3
fatal: reference is not a tree: 274829b9fc8b9ec375adcda95dfa498f20cb95f3
The command "git checkout -qf 274829b9fc8b9ec375adcda95dfa498f20cb95f3" failed and exited with 128 during .
Your build has been stopped.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Jun 19, 2019

⌛ Testing commit 45bb409 with merge 529d0931178b8cb96ad2f5d61d7165f74aaa43bd...

@Centril
Copy link
Contributor

Centril commented Jun 19, 2019

@bors retry yielding to r0llup

bors added a commit that referenced this pull request Jun 19, 2019
Rollup of 11 pull requests

Successful merges:

 - #61505 (Only show methods that appear in `impl` blocks in the Implementors sections of trait doc pages)
 - #61701 (move stray run-pass const tests into const/ folder)
 - #61748 (Tweak transparent enums and unions diagnostic spans)
 - #61802 (Make MaybeUninit #[repr(transparent)])
 - #61839 (ci: Add a script for generating CPU usage graphs)
 - #61842 (Remove unnecessary lift calls)
 - #61843 (Turn down the myriad-closures test)
 - #61896 (rustc_typeck: correctly compute `Substs` for `Res::SelfCtor`.)
 - #61898 (syntax: Factor out common fields from `SyntaxExtension` variants)
 - #61938 (create an issue for miri even in status test-fail)
 - #61941 (Preserve generator and yield source for error messages)

Failed merges:

r? @ghost
@rust-highfive
Copy link
Collaborator

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
Cloning into 'rust-lang/rust'...
travis_time:end:02f048be:start=1560919283292553262,finish=1560919289598587686,duration=6306034424
$ cd rust-lang/rust
$ git checkout -qf 529d0931178b8cb96ad2f5d61d7165f74aaa43bd
fatal: reference is not a tree: 529d0931178b8cb96ad2f5d61d7165f74aaa43bd
The command "git checkout -qf 529d0931178b8cb96ad2f5d61d7165f74aaa43bd" failed and exited with 128 during .
Your build has been stopped.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

bors added a commit that referenced this pull request Jun 19, 2019
Rollup of 11 pull requests

Successful merges:

 - #61505 (Only show methods that appear in `impl` blocks in the Implementors sections of trait doc pages)
 - #61701 (move stray run-pass const tests into const/ folder)
 - #61748 (Tweak transparent enums and unions diagnostic spans)
 - #61802 (Make MaybeUninit #[repr(transparent)])
 - #61839 (ci: Add a script for generating CPU usage graphs)
 - #61842 (Remove unnecessary lift calls)
 - #61843 (Turn down the myriad-closures test)
 - #61896 (rustc_typeck: correctly compute `Substs` for `Res::SelfCtor`.)
 - #61898 (syntax: Factor out common fields from `SyntaxExtension` variants)
 - #61938 (create an issue for miri even in status test-fail)
 - #61941 (Preserve generator and yield source for error messages)

Failed merges:

r? @ghost
@bors bors merged commit 45bb409 into rust-lang:master Jun 19, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 8, 2019
Add Iterator comparison methods that take a comparison function

This PR adds `Iterator::{cmp_by, partial_cmp_by, eq_by, ne_by, lt_by, le_by, gt_by, ge_by}`. We already have `Iterator::{cmp, partial_cmp, ...}` which are less general (but not any simpler) than the ones I'm proposing here.

I'm submitting this PR now because rust-lang#61505 has been merged, so this change should not have a noticeable effect on the `Iterator` docs page size.

The diff is quite messy, here's what I changed:
- The logic of `cmp` / `partial_cmp` / `eq` is moved to `cmp_by` / `partial_cmp_by` / `eq_by` respectively, changing `x.cmp(&y)` to `cmp(&x, &y)` in the `cmp` method where `cmp` is the given comparison function (and similar for `partial_cmp_by` and `eq_by`).
- `ne_by` / `lt_by` / `le_by` / `gt_by` / `ge_by` are each implemented in terms of one of the three methods above.
- The existing comparison methods are each forwarded to their `_by` counterpart, passing one of `Ord::cmp` / `PartialOrd::partial_cmp` / `PartialEq::eq` as the comparison function.

The corresponding `_by_key` methods aren't included because they're not as fundamental as the `_by` methods and can easily be implemented in terms of them. Is that reasonable, or would adding the `_by_key` methods be desirable for the sake of completeness?

I didn't add any tests – I couldn't think of any that weren't already covered by our existing tests. Let me know if there's a particular test that would be useful to add.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants