-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Trait impl show docs #51885
Trait impl show docs #51885
Conversation
The job Click to expand the log.
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 |
7bf660c
to
e5d2743
Compare
The job Click to expand the log.
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 |
e5d2743
to
b50eea1
Compare
The job Click to expand the log.
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 |
b50eea1
to
5b8006c
Compare
Would it be possible to only show the impl-specific docs on the trait page? For example, if the impl provides docs of its own we can print that regardless, but otherwise it would only print the trait's docs if it were on the struct page. (Which i think would be covered by |
By default docs are hidden and I think that the issue wanted to have the trait's docs. Or at least that's how I understood it and I like it as is. :) |
@llogiq What do you think about my suggestion? |
@QuietMisdreavus I like it! The general trait docs are already in the header, and only showing the |
☔ The latest upstream changes (presumably #52123) made this pull request unmergeable. Please resolve the merge conflicts. |
5b8006c
to
1f2e733
Compare
Ping from triage, @QuietMisdreavus. This PR requires your review, I think. |
@GuillaumeGomez Does this include my suggestion about not printing the "default" docs when on the trait page, now that @llogiq has agreed? |
I think it does print it. If you want me to remove it, it'll take a bit of time but I'll try to update it in the next week or so... |
Ping from triage @QuietMisdreavus! This PR needs your review. |
Still waiting on @GuillaumeGomez to address the last review comment. |
Ping from triage @GuillaumeGomez! It's been a while since we heard from you, will you have time to work on this again? |
I'll *soon*. |
Triage: @GuillaumeGomez Have you been able to get back to this? |
1f2e733
to
0c4c647
Compare
I did but debate still ongoing between @QuietMisdreavus and me. |
The job Click to expand the log.
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 |
63eb49c
to
941a692
Compare
No more default docs. |
Should we merge then? |
I have another suggestion. Can we only show methods that have impl-specific docs on them? In other words, in that Clone example above, instead of showing all those To say it another way, consider this example: pub trait SomeTrait {
fn one(&self);
fn two(&self);
}
pub struct SomeStruct;
impl SomeTrait for SomeStruct {
/// Here are some impl-specific docs!
fn one(&self) {}
// (note that this function is not specially documented!)
fn two(&self) {}
} On the page for |
I'd worry somewhat that users might think that they can't call two then, especially new users. Could we maybe have the non-documented options hidden by default in |
☔ The latest upstream changes (presumably #53884) made this pull request unmergeable. Please resolve the merge conflicts. |
@Mark-Simulacrum Not sure to follow you: what options are you talking about? |
er, I meant consts/items, sorry. |
Ok, I'll add that as well. |
b2174fb
to
d540914
Compare
@Mark-Simulacrum After taking a look, it seems like another big change. I'd prefer having it in another PR. This one is already fixing two issues and has been open for quite some time already. I'd prefer avoiding delaying it any further. |
I'm not exactly sure about the current state of this PR, but it was my loose impression that my concerns were about changes that this PR made. If they're not, then that's fine -- we can open an issue instead -- but otherwise I'd rather not land this if it does in fact make things worse in what I raised concerns about. |
Not worse but not (much?) better either. I'll open an issue so I can work on it separately. |
@bors: r=Mark-Simulacrum,QuietMisdreavus |
📌 Commit d540914 has been approved by |
…mulacrum,QuietMisdreavus Trait impl show docs Fixes #51834. <img width="1440" alt="screen shot 2018-06-29 at 00 14 33" src="https://user-images.githubusercontent.com/3050060/42063323-6e6e8cc8-7b31-11e8-88ef-4dd2229df76c.png"> (You can see both commit changes in the screenshot 😄) r? @QuietMisdreavus
☀️ Test successful - status-appveyor, status-travis |
…tems, r=QuietMisdreavus Hide default impls items Follow up of rust-lang#51885. Fixes rust-lang#54025. cc @Mark-Simulacrum r? @QuietMisdreavus And screenshots of course: <img width="1440" alt="screen shot 2018-09-12 at 23 30 35" src="https://user-images.githubusercontent.com/3050060/45454424-1ff8d500-b6e4-11e8-9257-030322495d58.png"> <img width="1440" alt="screen shot 2018-09-12 at 23 30 42" src="https://user-images.githubusercontent.com/3050060/45454431-2424f280-b6e4-11e8-8d65-db0d85ac18f0.png">
Fixes #51834.
(You can see both commit changes in the screenshot 😄)
r? @QuietMisdreavus