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 missing type links in documentation #62120

Merged
merged 1 commit into from
Jul 26, 2019

Conversation

GuillaumeGomez
Copy link
Member

r? @rust-lang/docs

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2019
@rust-highfive

This comment has been minimized.

//! operations such as [`mem::swap`]:
//! and common smart-pointer types such as [`Box<T>`] and `&mut T` allow replacing and
//! moving the values they contain: you can move out of a [`Box<T>`], or you can use
//! [`mem::swap`]. [`Pin<P>`] wraps a pointer type `P`, so [`Pin`]`<`[`Box<T>`]`>` functions much
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if intra docs understood the concept of a type constructor and gave you two links automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Type constructor? You're talking about Pin<Box<T>> I guess? If so, we can't be sure you want two links or just one. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Two links, one to Pin and one to Box. We don't want a link to T because it's a type variable (well... it might be useful actually...).

Copy link
Member Author

Choose a reason for hiding this comment

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

It *could*, we can't guess it automatically unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

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

@GuillaumeGomez Can you not use resolution info check whether T comes from a type variable? For Pin<Box<T>> we can at least determine syntactically that Pin and Box are type constructors and that Pin, Box, and T are separate entities.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but we can't be sure we're referring to an actual type or not something we just made up in the explanation two lines ago. :-/

@GuillaumeGomez GuillaumeGomez force-pushed the add-missing-type-links branch from 14b20f0 to a495a46 Compare June 26, 2019 08:32
@rust-highfive

This comment has been minimized.

@GuillaumeGomez

This comment has been minimized.

@GuillaumeGomez

This comment has been minimized.

@GuillaumeGomez GuillaumeGomez force-pushed the add-missing-type-links branch 7 times, most recently from 7f5df23 to 3a5eb6e Compare July 4, 2019 16:47
@GuillaumeGomez GuillaumeGomez force-pushed the add-missing-type-links branch from 3a5eb6e to c8aa3c1 Compare July 4, 2019 20:30
@QuietMisdreavus
Copy link
Member

@rust-lang/docs I'm starting to question the utility of having every single reference to some type have a hyperlink. If something was linked the first time it was mentioned, what's the use of linking it four times in the same paragraph? It's starting to feel like it's just adding noise to the docs and extra bytes to the page size.

This PR still seems like it introduces some new type links, so it's still somewhat useful, but paragraphs that have three links to Pin, Box, or drop feel like they're adding more noise for not much utility.

@shepmaster
Copy link
Member

If something was linked the first time it was mentioned

I am in agreement with this guideline (whether I follow it or not is a different question...).

How outrageous would it be for rustdoc to enforce it? That is, only use the first unique (link text, URL) pair in some container (say inside a {h1, h2, ... h6} delimited section)?

@GuillaumeGomez
Copy link
Member Author

I prefer adding links for every types for a few reasons:

  • it keeps the same appearance
  • it's more convenient and don't force the user to look for the first occurrence
  • it prevents potential "conflicts" between talking about a concept and a type in the same place (it happened a few times)

For those reasons, it seems good to me to make link for every appearance of a type that isn't Self.

@hdhoang
Copy link
Contributor

hdhoang commented Jul 26, 2019

ping from triage @Centril, any updates on this?

@Centril
Copy link
Contributor

Centril commented Jul 26, 2019

r? @Centril @bors r+ rollup

I'm in agreement with @GuillaumeGomez about the utility here. While it might not be as aesthetically pleasing, the functionality / utility provided by links is more important. If you disagree feel free to unapprove.

@bors
Copy link
Contributor

bors commented Jul 26, 2019

📌 Commit c8aa3c1 has been approved by Centril

@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 Jul 26, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 26, 2019
…ks, r=Centril

Add missing type links in documentation

r? @rust-lang/docs
Centril added a commit to Centril/rust that referenced this pull request Jul 26, 2019
…ks, r=Centril

Add missing type links in documentation

r? @rust-lang/docs
bors added a commit that referenced this pull request Jul 26, 2019
Rollup of 22 pull requests

Successful merges:

 - #62084 (allow clippy::unreadable_literal in unicode tables)
 - #62120 (Add missing type links in documentation)
 - #62310 (Add missing doc links in boxed module)
 - #62421 (Introduce `as_deref` to Option)
 - #62583 (Implement Unpin for all raw pointers)
 - #62692 (rustc: precompute the largest Niche and store it in LayoutDetails.)
 - #62801 (Remove support for -Zlower-128bit-ops)
 - #62828 (Remove vector fadd/fmul reduction workarounds)
 - #62862 (code cleanup)
 - #62904 (Disable d32 on armv6 hf targets)
 - #62907 (Initialize the MSP430 AsmParser)
 - #62956 (Implement slow-path for FirstSets::first)
 - #62963 (Allow lexer to recover from some homoglyphs)
 - #62964 (clarify and unify some type test names)
 - #62970 (ci: gate toolstate repo pushes on the TOOLSTATE_PUBLISH envvar)
 - #62980 (std: Add more accessors for `Metadata` on Windows)
 - #62983 (Remove needless indirection through Rc)
 - #62985 (librustc_errors: Support ui-testing flag in annotate-snippet emitter)
 - #63002 (error_index_generator should output stdout/stderr when it panics.)
 - #63004 (Add test for issue-54062)
 - #63007 (ci: debug network failures while downloading awscli from PyPI)
 - #63009 (Remove redundant `mut` from variable declaration.)

Failed merges:

r? @ghost
@bors bors merged commit c8aa3c1 into rust-lang:master Jul 26, 2019
@GuillaumeGomez GuillaumeGomez deleted the add-missing-type-links branch July 27, 2019 09:57
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