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

Fix convert module's documentation links #59923

Merged
merged 7 commits into from
May 16, 2019

Conversation

czipperz
Copy link
Contributor

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 12, 2019
@@ -213,7 +213,7 @@ pub trait AsMut<T: ?Sized> {
///
/// # Generic Implementations
///
/// - [`From<T>`]` for U` implies `Into<U> for T`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep this as is and let From<T> be linked to From.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation is inconsistent in this regard. Some places it uses

[`From<T>`]` ...`

other times it uses

[`From`]`<T> ...`

Why do you prefer the former? The latter reduces copy paste of the link destinations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise it looks like this: From<T> which doesn't look great.

Copy link
Contributor Author

@czipperz czipperz Apr 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See convert module it looks fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should I fix this line too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think it looks weird; there's a space in-between that shouldn't be there, but I leave it to Steve.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a good convention, and I agree the space is unfortunate. Maybe @rust-lang/rustdoc can weigh in?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to prefer how @czipperz wrote it.

Copy link
Contributor Author

@czipperz czipperz Apr 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the spacing should theoretically be more messed up in the later as @Centril pointed out, it really looks more messed up the former when rendered by rustdoc. Strange. image

Copy link
Contributor Author

@czipperz czipperz Apr 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This style (of putting <T> not inside the link) is much less popular than the other way around. Before this pr, its used 40 times throughout the code base vs 168 for the other style.

Edit: my regex was wrong

Here's an example from Weak's documentation:
image

@GuillaumeGomez
Copy link
Member

Why removing the links on Into btw? It's linking to the wrong thing?

@czipperz
Copy link
Contributor Author

@GuillaumeGomez turns out they aren't broken. My bad. But it is standard to not link to Self

@Dylan-DPC-zz
Copy link

ping from triage @steveklabnik any updates?

@steveklabnik
Copy link
Member

Sorry for taking so long here; I took two weeks off. Thank you for this!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented May 15, 2019

📌 Commit 1f5d510 has been approved by steveklabnik

@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 May 15, 2019
Centril added a commit to Centril/rust that referenced this pull request May 15, 2019
…teveklabnik

Fix convert module's documentation links

r? @steveklabnik
bors added a commit that referenced this pull request May 15, 2019
Rollup of 9 pull requests

Successful merges:

 - #59825 (string: implement From<&String> for String)
 - #59923 (Fix convert module's documentation links)
 - #60691 (Include expression to wait for to the span of Await)
 - #60763 (Move token tree related lexer state to a separate struct)
 - #60769 (Update rustc book CLI docs.)
 - #60811 (Bump measureme dependency to 0.3)
 - #60816 (README.md: Mention MSVC 2017+, not 2013(!))
 - #60827 (Use `Symbol` more in lint APIs)
 - #60851 (Move `box` from the stable keyword to unstable keywords list)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request May 15, 2019
…teveklabnik

Fix convert module's documentation links

r? @steveklabnik
Centril added a commit to Centril/rust that referenced this pull request May 16, 2019
…teveklabnik

Fix convert module's documentation links

r? @steveklabnik
bors added a commit that referenced this pull request May 16, 2019
Rollup of 6 pull requests

Successful merges:

 - #59825 (string: implement From<&String> for String)
 - #59923 (Fix convert module's documentation links)
 - #60691 (Include expression to wait for to the span of Await)
 - #60769 (Update rustc book CLI docs.)
 - #60816 (README.md: Mention MSVC 2017+, not 2013(!))
 - #60851 (Move `box` from the stable keyword to unstable keywords list)

Failed merges:

r? @ghost
@bors bors merged commit 1f5d510 into rust-lang:master May 16, 2019
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants