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

Use intra-doc links in core::iter module #74300

Merged
merged 8 commits into from
Jul 18, 2020
Merged

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Jul 13, 2020

This will make core::iter doc depend less on std doc.

@rust-highfive
Copy link
Collaborator

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 13, 2020
@lcnr
Copy link
Contributor

lcnr commented Jul 13, 2020

This would result in dead links in std due to #65983, also see #73579 (comment) where I personally first noticed this problem.

/// [`sum`]: ../../std/iter/trait.Sum.html#tymethod.sum
/// [`FromIterator`]: ../../std/iter/trait.FromIterator.html
/// [`Iterator::sum`]: ../../std/iter/trait.Iterator.html#method.sum
/// [`sum`]: #tymethod.sum
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this not have an equivalent?

Copy link
Contributor Author

@tesuji tesuji Jul 14, 2020

Choose a reason for hiding this comment

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

This fragment link is fine as the sum method referred belongs to Sum trait.

Copy link
Member

Choose a reason for hiding this comment

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

This is #51284, which is a project all on its own. If @lzutao checked that it works I'm fine with it.

Copy link
Member

Choose a reason for hiding this comment

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

Can this not be Sum::sum or Self::sum?

Copy link
Contributor Author

@tesuji tesuji Jul 18, 2020

Choose a reason for hiding this comment

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

Is it documented in Intra RFC? That's new to me!

@tesuji
Copy link
Contributor Author

tesuji commented Jul 14, 2020

@lcnr I don't quite sure there are any broken links as linkchecker doesn't warn.
Also I don't use cross-crate links (I think), only crate:: prefix links.
cc @jyn514 to make sure.

@jyn514
Copy link
Member

jyn514 commented Jul 14, 2020

I'm pretty sure core is re-exported as part of std, so that could break things. You should check locally to be sure.

@jyn514
Copy link
Member

jyn514 commented Jul 14, 2020

As a side note, #73101 is almost ready to be merged, maybe 1 or 2 more weeks.

@tesuji
Copy link
Contributor Author

tesuji commented Jul 14, 2020

Checked locally and ran linkchecker. I don't see any broken links
in both std and core.

@lcnr
Copy link
Contributor

lcnr commented Jul 14, 2020

Screenshot from 2020-07-14 10-12-46

@tesuji
Copy link
Contributor Author

tesuji commented Jul 14, 2020

Pardon me?
Edit: Oh, I see it now. Option is not linked. I will see what I can do to fix the links.

@tesuji
Copy link
Contributor Author

tesuji commented Jul 14, 2020

After experimenting, I observed two things:

  • Replacing [`Option`] in core::iter with crate::option::Option broke the link.
  • Letting rustdoc handle Option link (no manually specify link), rustdoc doesn't make link or warn about that.

The broke links are detected by linkchecker. I don't know if there are more empty-links,
but this PR shouldn't broke links.

@jyn514
Copy link
Member

jyn514 commented Jul 17, 2020

This would result in dead links in std due to #65983, also see #73579 (comment) where I personally first noticed this problem.

#73101 just landed so this should be fixed once you rebase 😄 You'll need to use x.py doc --stage 1 to get the changes.

@tesuji
Copy link
Contributor Author

tesuji commented Jul 17, 2020

Rebased. Looks like stage1 rustdoc fixes the Option link in core::iter .
r? @jyn514

@rust-highfive rust-highfive assigned jyn514 and unassigned withoutboats Jul 17, 2020
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Looks great! It's good to see how this is used in the wild, it gave me ideas for more features to add 😆 I had one small nit but everything else is informative or notes to myself.

src/libcore/iter/traits/iterator.rs Outdated Show resolved Hide resolved
/// [`Option<T>`]: ../../std/option/enum.Option.html
/// [`Some`]: ../../std/option/enum.Option.html#variant.Some
/// [`None`]: ../../std/option/enum.Option.html#variant.None
/// [`Option<T>`]: Option
Copy link
Member

Choose a reason for hiding this comment

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

This is #62834. This is a good workaround in the meantime though.

Copy link
Contributor

Choose a reason for hiding this comment

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

But why not just use Option rather than Option<T>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Option<T> seems to be clearer for beginners/first readers of Iterator.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could use Option<T> in the book, but I don't think beginners get here easily.

@@ -1366,8 +1350,7 @@ pub trait Iterator {
/// [`Some(T)`] again. `fuse()` adapts an iterator, ensuring that after a
/// [`None`] is given, it will always return [`None`] forever.
///
/// [`None`]: ../../std/option/enum.Option.html#variant.None
/// [`Some(T)`]: ../../std/option/enum.Option.html#variant.Some
/// [`Some(T)`]: Some
Copy link
Member

Choose a reason for hiding this comment

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

I've seen this quite a few times now, maybe it would be a good feature for intra-doc links to add, same as #62834?

@jyn514
Copy link
Member

jyn514 commented Jul 17, 2020

ran linkchecker. I don't see any broken links

#74185 (comment). Maybe we should add deny(intra_doc_link_resolutuion_failure) now that cross crate re-exports work?

@jyn514
Copy link
Member

jyn514 commented Jul 17, 2020

Maybe we should add deny(intra_doc_link_resolutuion_failure) now that cross crate re-exports work?

It turns out this will be on by default starting in #71670. But it doesn't hurt to have it on now.

@jyn514
Copy link
Member

jyn514 commented Jul 17, 2020

r=me, but I don't have bors permissions until rust-lang/team#384 lands.

@jyn514
Copy link
Member

jyn514 commented Jul 17, 2020

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 17, 2020

@jyn514: 🔑 Insufficient privileges: Not in reviewers

@lcnr
Copy link
Contributor

lcnr commented Jul 17, 2020

@bors r=jyn514 rollup=always

@bors
Copy link
Contributor

bors commented Jul 17, 2020

📌 Commit 5ffdd7c has been approved by jyn514

@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 17, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 17, 2020
Use intra-doc links in core::iter module

This will make core::iter doc depend less on std doc.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2020
…arth

Rollup of 18 pull requests

Successful merges:

 - rust-lang#71670 (Enforce even more the code blocks attributes check through rustdoc)
 - rust-lang#73930 (Make some Option methods const)
 - rust-lang#74009 (Fix MinGW `run-make-fulldeps` tests)
 - rust-lang#74056 (Add Arguments::as_str().)
 - rust-lang#74169 (Stop processing unreachable blocks when solving dataflow)
 - rust-lang#74251 (Teach bootstrap about target files vs target triples)
 - rust-lang#74288 (Fix src/test/run-make/static-pie/test-aslr.rs)
 - rust-lang#74300 (Use intra-doc links in core::iter module)
 - rust-lang#74364 (add lazy normalization regression tests)
 - rust-lang#74368 (Add CSS tidy check)
 - rust-lang#74394 (Remove leftover from emscripten fastcomp support)
 - rust-lang#74411 (Don't assign `()` to `!` MIR locals)
 - rust-lang#74416 (Use an UTF-8 locale for the linker.)
 - rust-lang#74424 (Move hir::Place to librustc_middle/hir)
 - rust-lang#74428 (docs: better demonstrate that None values are skipped as many times a…)
 - rust-lang#74438 (warn about uninitialized multi-variant enums)
 - rust-lang#74440 (Fix Arc::as_ptr docs)
 - rust-lang#74452 (intra-doc links: resolve modules in the type namespace)

Failed merges:

r? @ghost
@bors bors merged commit c9010d6 into rust-lang:master Jul 18, 2020
@tesuji tesuji deleted the iterator-intra branch July 18, 2020 02:12
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
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.

9 participants