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

Document direct implementations on type aliases. #42027

Merged
merged 1 commit into from
Jun 9, 2017
Merged

Document direct implementations on type aliases. #42027

merged 1 commit into from
Jun 9, 2017

Conversation

mjkillough
Copy link
Contributor

This improves #32077, but is not a complete fix.

For a type alias type NewType = AliasedType, it will include any impl NewType and impl Trait for NewType blocks in the documentation for NewType.

A complete fix would include the implementations from the aliased type in the type alias' documentation, so that users have a complete picture of methods that are available on the alias. However, to do this properly would require a fix for #14072, as the alias may affect the type parameters of the type alias, making the documentation difficult to understand. (That is, for type Result = std::result::Result<(), ()> we would ideally show documentation for impl Result<(), ()>, rather than generic documentation for impl<T, E> Result<T, E>).

I think this improvement is worthwhile, as it exposes implementations which are not currently documented by rustdoc. The documentation for the implementations on the aliased type are still accessible by clicking through to the docs for that type. (Although perhaps it's now less obvious to the user that they should click-through to get there).

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@arielb1 arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2017
@Mark-Simulacrum
Copy link
Member

@steveklabnik This needs a review; could you take a look?

@steveklabnik
Copy link
Member

@GuillaumeGomez @QuietMisdreavus what do you think?

@QuietMisdreavus
Copy link
Member

Looks good to me, save for one request: Could you add the associated item headers to the sidebar, too? The meat of it should be in the impl<'a> fmt::Display for Sidebar<'a> in src/librustdoc/html/render.rs (should start around line 3089). You'd need to add a fn is_typedef to Item in src/librustdoc/clean/mod.rs (the impl block should start around line 273) and compare against ItemType::Typedef the same way the other is_thing functions do, to open the if block the same way. When matching, you can add an arm for clean::TypedefItem(..) and add the heading text "Type Definition" to match the header at the top of the main page. Farther down, you'll have to add a sidebar_typedef function and call it in that Sidebar impl, but it should look almost identical to sidebar_primitive since the only headings getting added are through the associated items.

This way we can make sure the new headings don't get left out of the sidebar. If you run into trouble adding these, let me know. Thanks!

@GuillaumeGomez
Copy link
Member

I think @QuietMisdreavus said everything that was to be said. :)

@shepmaster shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2017
@aidanhs
Copy link
Member

aidanhs commented Jun 1, 2017

Hi @mjkillough, have you had a chance to have a look at the review comment?

@QuietMisdreavus
Copy link
Member

If you'd like, I can write in the suggestion I made; it looks like the "Allow edits from maintainers" box is checked, so that shouldn't be too difficult. That way we can still get this merged.

@mjkillough
Copy link
Contributor Author

Thanks a lot for the very useful comment! I hadn't spotted that I needed to add these to the sidebar too.

I'd be happy to make the suggested changes, but I'll be on holiday for the next week. If you'd like to get this merged sooner, please feel free to update the pull request.

@GuillaumeGomez
Copy link
Member

@mjkillough: We're not in a hurry, don't worry. Take your time to finish this and then we'll merge.

@mjkillough
Copy link
Contributor Author

@QuietMisdreavus - Thanks again for the detailed review comment! I've made the suggested changed and updated my test accordingly.

Please can you take another look?

@QuietMisdreavus
Copy link
Member

Looks great, glad you were able to get it working!

One last thing and we'll get this squared away: Could you squash these commits down? That way we can keep the commit history clean.

@GuillaumeGomez
Copy link
Member

This improves #32077, but is not a complete fix. For a type alias `type
NewType = AliasedType`, it will include any `impl NewType` and `impl
Trait for NewType` blocks in the documentation for `NewType`.

A complete fix would include the implementations from the aliased type
in the type alias' documentation, so that users have a complete
picture of methods that are available on the alias. However, to do this
properly would require a fix for #14072, as the alias may affect the
type parameters of the type alias, making the documentation difficult to
understand. (That is, for `type Result = std::result::Result<(), ()>` we
would ideally show documentation for `impl Result<(), ()>`, rather than
generic documentation for `impl<T, E> Result<T, E>`).

I think this improvement is worthwhile, as it exposes implementations
which are not currently documented by rustdoc. The documentation for the
implementations on the aliased type are still accessible by clicking
through to the docs for that type. (Although perhaps it's now less
obvious to the user that they should click-through to get there).
@mjkillough
Copy link
Contributor Author

@QuietMisdreavus / @GuillaumeGomez - Thanks for the quick response! I've now squashed and rebased on top of master.

@QuietMisdreavus
Copy link
Member

Thanks so much!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 9, 2017

📌 Commit 2da3501 has been approved by QuietMisdreavus

@bors
Copy link
Contributor

bors commented Jun 9, 2017

⌛ Testing commit 2da3501 with merge 2416e22...

bors added a commit that referenced this pull request Jun 9, 2017
Document direct implementations on type aliases.

This improves #32077, but is not a complete fix.

For a type alias `type NewType = AliasedType`, it will include any `impl NewType` and `impl
Trait for NewType` blocks in the documentation for `NewType`.

A complete fix would include the implementations from the aliased type in the type alias' documentation, so that users have a complete picture of methods that are available on the alias. However, to do this properly would require a fix for #14072, as the alias may affect the type parameters of the type alias, making the documentation difficult to understand. (That is, for `type Result = std::result::Result<(), ()>` we would ideally show documentation for `impl Result<(), ()>`, rather than generic documentation for `impl<T, E> Result<T, E>`).

I think this improvement is worthwhile, as it exposes implementations which are not currently documented by rustdoc. The documentation for the implementations on the aliased type are still accessible by clicking through to the docs for that type. (Although perhaps it's now less obvious to the user that they should click-through to get there).
@bors
Copy link
Contributor

bors commented Jun 9, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing 2416e22 to master...

@bors bors merged commit 2da3501 into rust-lang:master Jun 9, 2017
@mjkillough
Copy link
Contributor Author

Thanks again for all the help!

@Hoverbear
Copy link
Contributor

Thank you all for fixing this! =D Can't wait to see it released -- have a crate that would benefit from this. Are we looking at a 12 week turn around on release?

@QuietMisdreavus
Copy link
Member

Since the new stable just released, that's about how long it will take to get to a stable release. However, it should also appear on the next nightly, so if you're willing to use a nightly to render your docs, you can grab tomorrow's nightly to test it out.

@mjkillough mjkillough deleted the typedef_assoc_items branch June 9, 2017 12:56
Hoverbear pushed a commit to Hoverbear/digitalocean that referenced this pull request Jun 14, 2017
We can do this because of rust-lang/rust#42027,
but it only works on nightly right now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.