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

Add links in docs for some primitive types #88469

Merged
merged 4 commits into from
Sep 5, 2021
Merged

Conversation

patrick-gu
Copy link
Contributor

@patrick-gu patrick-gu commented Aug 29, 2021

This pull request adds additional links in existing documentation of some of the primitive types.

Where items are linked only once, I have used the [link](destination) format. For items in std, I have linked directly to the HTML, since although the primitives are in core, they are not displayed on core documentation. I was unsure of what length I should keep lines of documentation to, so I tried to keep them within reason.

Additionally, I have avoided excessively linking to keywords like self when they are not relevant to the documentation. I can add these links if it would be an improvement.

I hope this can improve Rust. Please let me know if there's anything I did wrong!

@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 @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2021
/// or `.into_iter()` on your array. `[T; N]::map` is only necessary if you
/// really need a new array of the same size as the result. Rust's lazy
/// In many cases, you can instead use [`Iterator::map`] by calling [`.iter()`](slice::iter)
/// or [`.into_iter()`](IntoIterator::into_iter) on your array. `[T; N]::map` is only necessary
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should be linking each of these methods -- it feels a little excessive. The documentation for into_iter/iter isn't anything special either, so users are likely to not gain too much from it.

@@ -2,7 +2,7 @@

#[lang = "bool"]
impl bool {
/// Returns `Some(t)` if the `bool` is `true`, or `None` otherwise.
/// Returns <code>[Some]\(t)</code> if the `bool` is [`true`](keyword.true.html), or [`None`] otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

  • Linking true seems a little excessive
  • I don't think we need to link Some or None, those are similarly to true basic vocabulary terms

@Mark-Simulacrum
Copy link
Member

Going to also nominate for T-libs-api, not necessarily to discuss this particular PR but I think it'd be good to write up some policy similar to the doc-alias policy for the std-dev-guide on what we expect linked and not linked in docs. It seems clear that there is a spectrum between linking every Rust method/type reference and some subset thereof, and I doubt we can come up with anything too literal, but some guidance would be good I think.

@Mark-Simulacrum Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-nominated labels Aug 30, 2021
@patrick-gu
Copy link
Contributor Author

I understand that some of the links may have been excessive (especially true). Once linkage guidance has been decided, I can make the appropriate changes. Thank you for looking at this!

@bors
Copy link
Contributor

bors commented Sep 3, 2021

☔ The latest upstream changes (presumably #88618) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Some general thoughts on documentation:

  • All documentation exists in a context of what prior knowledge is already expected of the reader. This principle is why it's so frustratingly useless when documentation does this: https://doc.rust-lang.org/1.54.0/std/cell/struct.Cell.html#method.as_ptr. Whoever is far enough along in learning Rust that they are reading about Cell is guaranteed to already have basic concepts out of the way like how to assign a variable and how to make a method call. Notice how the example code in that link is 100% useless noise. There is absolutely opportunity on that method for example code to illustrate why you would want to call that specific method in real world usage; squandering it with how to make a method call is silly and lazy.

  • More documentation is not better. From the same page: https://doc.rust-lang.org/1.54.0/std/cell/struct.Cell.html#method.from_mut. See how the sentence just restates the method signature in a worse way. Someone felt compelled to add documentation under the presumable understanding that more documentation is always better; it is not. As part of producing documentation your job is to critically analyze what value the documentation is conveying to the reader.

  • More links are not better. This is a special case of the combination of the previous 2 points. If you feel compelled to add a link because a word exists that can conceivably be associated with another page, and not because you actually expect any reader to derive value from clicking your link, it is misguided. Documentation exists in a context of what the reader is already expected to know, like what true means by the time they make it to reading about unicode properties. You are responsible for evaluating whether the link is plausibly expected to deliver value for the reader. (There are other reasons to overlink, notably SEO, but it does not currently apply in Rust standard library documentation.)

  • Too many links are worse than no links. Reading perpetual link after link is exhausting and distracting. For every link that a reader passes as part of reading a block of prose, the reader is required to come to a decision about whether they want to click the link now, finish the sentence/paragraph and click the link then, or not click the link. Search exists (both rustdoc search and Google search) and is absolutely sufficient for items that a small fraction of readers may not already be familiar with.

  • Linked structured content is greatly preferred over links embedded into unstructured text. This is in my experience but I'm sure we could get analytics for the standard library documentation with some effort. In the case that prose is discussing types that are part of a function signature, linking the type names in the prose is useless because the great majority of readers would be clicking the other link via the rustdoc-generated function signature either way.

I didn't read through your changes in the PR in detail to categorize the good and bad ones, but I can take a look after you've updated with the above in mind.

@rust-lang/libs-api in case anyone wants to comment on the guidance.

@patrick-gu
Copy link
Contributor Author

I completely understand that quality of linking would be better than just the quantity.

I've generally tried to remove the unhelpful/repeated links. I have still left some links to common items, such as:

  • Linking to if, true, and false in bool, since those are closely related topics.
  • Linking to bool from primitive references, as they may not be obvious to someone who is reading the documentation for primitives.
  • PartialOrd and Ord from tuples, as, again, they may not be self-explanatory.

I'm not sure if all of the links remaining are necessary, especially with to_string. Please let me know if there are any more changes I should make. Thank you.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@dtolnay
Copy link
Member

dtolnay commented Sep 4, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Sep 4, 2021

📌 Commit 7c32b58 has been approved by dtolnay

@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 Sep 4, 2021
@bors
Copy link
Contributor

bors commented Sep 5, 2021

⌛ Testing commit 7c32b58 with merge 0961e68...

@bors
Copy link
Contributor

bors commented Sep 5, 2021

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 0961e68 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 5, 2021
@bors bors merged commit 0961e68 into rust-lang:master Sep 5, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants