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

Hide trait default methods #62734

Merged
merged 3 commits into from
Aug 29, 2019
Merged

Conversation

GuillaumeGomez
Copy link
Member

Fixes #62499.

However, the question remains: do we want to extend it to this point or not?

r? @QuietMisdreavus

@jonas-schievink jonas-schievink added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 26, 2019
@wirelessringo
Copy link

Ping from triage. @QuietMisdreavus any updates on this? Thanks.

@GuillaumeGomez
Copy link
Member Author

cc @ollie27

@JohnTitor
Copy link
Member

Ping from triage: @QuietMisdreavus do you have time to review?

@fmckeogh
Copy link
Member

Second ping from triage! Per procedure a new reviewer from the rustic team will be assigned.

r? @onur

@GuillaumeGomez
Copy link
Member Author

r? @Mark-Simulacrum

@Alexendoo
Copy link
Member

Ping from triage, any updates @Mark-Simulacrum?

@Mark-Simulacrum
Copy link
Member

From a code perspective my only concern is that I'm not quite sure if this will affect overridden defaults (i.e., the trait method has a default impl but the impl Trait for Foo also implements that method); if that works as before and is shown then I think the code is fine by me.

From a policy perspective, probably the best approach here is to FCP the change? I'm not sure if rustdoc team has a firm policy in place. If you think it's fine to just land this without team approval then I'm fine with that, r=me.

@GuillaumeGomez
Copy link
Member Author

Normally the tests should have checked that the overwrote methods with documentation are not hidden. Unless I missed something? Also, we only apply FCP for "big" changes, this one is pretty small in itself. That'd maybe deserve a talk when other rustdoc members are back. Meanwhile I'll r+ this PR but don't hesitate to r- if you feel like we should wait or anything seems not good and thanks for the review!

@bors: r=Mark-Simulacrum rollup

@bors
Copy link
Contributor

bors commented Aug 29, 2019

📌 Commit f7656b6 has been approved by Mark-Simulacrum

@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 Aug 29, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 29, 2019
…, r=Mark-Simulacrum

Hide trait default methods

Fixes rust-lang#62499.

However, the question remains: do we want to extend it to this point or not?

r? @QuietMisdreavus
bors added a commit that referenced this pull request Aug 29, 2019
Rollup of 4 pull requests

Successful merges:

 - #62734 (Hide trait default methods)
 - #63953 (bootstrap: allow specifying mirror for bootstrap compiler download.)
 - #63956 (rustc: Handle modules in "fat" LTO more robustly)
 - #63979 (std: Remove the `wasm_syscall` feature)

Failed merges:

r? @ghost
@bors bors merged commit f7656b6 into rust-lang:master Aug 29, 2019
@GuillaumeGomez GuillaumeGomez deleted the hide-default-methods branch August 30, 2019 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

rustdoc: Trait methods with default implementations are never hidden
10 participants