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

Attempt to fix all documentation warnings from rustdoc. #1574

Merged
merged 12 commits into from
May 6, 2019

Conversation

najamelan
Copy link
Contributor

I attempted to fix all documentation warnings in futures-rs. All of these refer to failing cross-
references. Note that it's hard to verify as there is still alot of warnings from dependencies
like rand and from what I suspect to be std, but it's unclear, since rustdoc doesn't mark the
origin of the warnings. See: rust-lang/rust#59367 and rust-lang/rust#55907

The fixes here contain several TODO's, for two main reasons:

  • false positive warnings from rustdoc, where rustdoc generates a correct link but still issues
    a warning
  • places where I have been obliged to put a link to an html file because I didn't manage to make
    rustdoc generate a correct link from a path.

It would be nice if people verified that commits don't throw warnings before merging,
even in rustdoc. Especially so because rustdoc does not hide warnings in dependencies
right now, even when called with --no-deps. That means that any warnings thrown by
futures-rs will bother every single dev that has a (indirect) dependency on futures-rs
and runs rustdoc.

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I left comments to solve some TODO. And the cause of some errors was that I wrote the wrong link 😅.

futures-util/src/try_stream/mod.rs Outdated Show resolved Hide resolved
futures-util/src/stream/enumerate.rs Outdated Show resolved Hide resolved
futures-util/src/io/seek.rs Outdated Show resolved Hide resolved
futures-util/src/try_stream/mod.rs Outdated Show resolved Hide resolved
futures-util/src/try_stream/mod.rs Outdated Show resolved Hide resolved
@najamelan
Copy link
Contributor Author

Actually, intertwined with the hundreds of warnings from rand, there is another bunch from futures, so I will continue my quest

@najamelan
Copy link
Contributor Author

So, a status report:

  • links to stuff in the same crate seem to work with crate:: paths.
  • cross crate links are not functional at all. Some will just not link and throw warnings, some will link to eg. core::task::Waker when an entity with an identical name exists in core or std. Note that it doesn't look like this will be fixed in rustdoc anytime soon.

There is another bunch of warnings left, but as long as there is no clear solution for cross crate links, I don't see how to continue this:

  • Do we wait for docs.rs to work again and link there?
  • do we prefer to link to: rust-lang-nursery.github.io/futures-api-docs/0.3.0-alpha.15 but who is going to update the version numbers all the time?

@taiki-e
Copy link
Member

taiki-e commented May 1, 2019

(FYI, I opened #1578 to avoid all doc errors from rand.)

@taiki-e
Copy link
Member

taiki-e commented May 1, 2019

There is another bunch of warnings left, but as long as there is no clear solution for cross crate links, I don't see how to continue this:

I think it's difficult to fix all these warnings at this point.

futures-test/src/task/mod.rs Outdated Show resolved Hide resolved
futures-test/src/task/mod.rs Outdated Show resolved Hide resolved
@taiki-e
Copy link
Member

taiki-e commented May 3, 2019

Some updates and reports:


#1574 (comment)
One of the reasons why doc-link problems are troublesome is that it's hard to tell if it comes from a bug in rustdoc or simply because the link (path) is wrong. For example, the warnings in #1574 (comment) and #1574 (comment) were due to them specifying an old path.

Also, in this PR, I think that it is Ok to ignore the problem that the link does not work in the cross-crate (because as far as I know the main purpose of this PR is to fix warnings).

@najamelan
Copy link
Contributor Author

najamelan commented May 3, 2019 via email

I attempted to fix all documentation warnings in futures-rs. All of these refer to failing cross-
references. Note that it's hard to verify as there is still alot of warnings from dependencies
like rand and from what I suspect to be std, but it's unclear, since rustdoc doesn't mark the
origin of the warnings. See: rust-lang/rust#59367 and rust-lang/rust#55907

The fixes here contain several TODO's, for two main reasons:
- false positive warnings from rustdoc, where rustdoc generates a correct link but still issues
  a warning
- places where I have been obliged to put a link to an html file because I didn't manage to make
  rustdoc generate a correct link from a path.

It would be nice if people verified that commits don't throw warnings before merging,
even in rustdoc. Especially so because rustdoc does not hide warnings in dependencies
right now, even when called with `--no-deps`. That means that any warnings thrown by
futures-rs will bother every single dev that has a (indirect) dependency on futures-rs
and runs rustdoc.
TODO: note that paths like futures_core::task::Context actually get redirected to core::task::Context
in the redered docs. Is this desirable? If so, should we change the paths here?

Also, there is cross crate links in here. They are not going to work anytime soon. Do we put https links
in here? to here: https://rust-lang-nursery.github.io/futures-api-docs? The problem is these have a
version hardcoded in the url: 0.3.0-alpha.15 We could link to docs.rs, but currently that says:
docs.rs failed to build futures-preview-0.3.0-alpha.15 -> ok the reason seems to be that they are on
2019-04-17 which does still have futures-api unstable feature, so that should get solved.
…o clear solution...

when the build on docs.rs passes again, we could decide to link to docs.rs, until then I have no solution.
@najamelan
Copy link
Contributor Author

those commits are just rebase on latest master, fixing more warnings now

but it will fail to generate correct links when rustdoc is run
at the futures-rs level. Eg, the re-exports fail:
futures::channel::oneshot::Receiver
@najamelan
Copy link
Contributor Author

najamelan commented May 5, 2019

ok, I think that's if for now. There is a lot of warnings from std, and the ones in futures-channel I don't get right. They work fine when running rustdoc in the futures-channel crate, but not when running it for all futures-rs.

I did find cross crate links working, but only to the original, not to re-exports. Hence links to futures_core::... and not to futures::...

@cramertj cramertj merged commit b93b785 into rust-lang:master May 6, 2019
@cramertj
Copy link
Member

cramertj commented May 6, 2019

Thanks so much for the help! :)

@najamelan
Copy link
Contributor Author

@cramertj Thank you for all the work on async and other rust. If I knew the commits were not going to be squashed, I would have made a bit more of a clean history, but then again, it probably doesn't change much in real life!

@najamelan najamelan deleted the fix/doc_warnings branch May 6, 2019 23:22
@cramertj
Copy link
Member

cramertj commented May 6, 2019

No worries-- if I were bothered about it I would've squashed them myself. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants