General rustdoc improvements #450

dekellum · 2018-06-25T21:42:05Z

I started this general branch of rustdoc improvements as part of the effort to document for #339. A description of blocking in the runtime module warrants a summary of the feature and links to greater detail. In particular, 014229f promotes your (@carllerche) recent documentation of tokio-threadpool to rustdoc so I can reference it.

dekellum · 2018-06-27T18:29:45Z

I could use some high level guidance on this. In particular, where as promoting the newest tokio-threadpool crate documentation over the (now hidden, older) tokio::executor::thread_pool docs seems to add value (4076bea...992d4f7), resolving the apparent redundancy in places like with the reactor will require more effort:

https://docs.rs/tokio/0.1/tokio/reactor/index.html (more details, no link).
https://docs.rs/tokio-reactor/0.1/tokio_reactor/ (less details, links to above)

Current links:

22:README.md:[reactor]: https://docs.rs/tokio/0.1/tokio/reactor/index.html
1723:src/runtime/current_thread/mod.rs://! [reactor]: ../../reactor/struct.Reactor.html
1906:src/runtime/mod.rs://! [reactor]: ../reactor/struct.Reactor.html

tokio-fs is in a slightly different state:

https://docs.rs/tokio/0.1/tokio/fs/index.html (less details, no link, incomplete re-exports)
https://docs.rs/tokio-fs/0.1.0/tokio_fs/ (0.1.1 failed on docs.rs, more details, no link)

I suspect moving all the docs to the associated split-out crates with only links from the tokio crate would be best, and the sooner the better, as it would be less confusing to users in current versions, without any code changes. This can be done in advance of deprecating the re-exports in tokio, at least where they are not essential or proving difficult to maintain.

To reiterate, the reason I've wandered down this particular rabbit hole is the desire to summarize on blocking in runtime (#339) by referencing details in tokio-threadpool and tokio-fs.

Thoughts on how to proceed?

carllerche · 2018-07-05T17:23:45Z

Thanks! Sorry for the delay. I have been out of town and am still catching up :)

carllerche · 2018-07-05T17:53:57Z

I don't think I have a great answer.

The facade strategy means that crates can be used standalone by lib authors, and application authors just depend on tokio and get everything pulled in.

The problem with documentation is that there are now two paths to get to docs depending on your use case. When looking at the sub crate doc, you need to be able to use the library standalone. When coming from the facade, you want to know how everything fits together. This is why the docs are a bit different. When documenting tokio::fs, you want to know how it works combined with tokio::runtime, but not if you go to tokio-fs the crate.

Because tokio::executor::thread_pool was deprecated, I don't expect "application devs" to need to know about the thread pool directly. Instead, they will use tokio::runtime.

carllerche

I added some thoughts inline.

carllerche · 2018-07-05T17:54:50Z

src/executor/mod.rs

@@ -44,79 +44,10 @@
 pub mod current_thread;

 #[deprecated(since = "0.1.8", note = "use tokio-threadpool crate instead")]
-#[doc(hidden)]
+/// Re-exports of [`tokio-threadpool`], deprecated in favor of the crate.


Could you clarify why you removed #[doc(hidden)]?

I removed #[doc(hidden)] so that the deprecation tag plus the short note and link I added to tokio-threadpool is visible. As of the last 0.1.7 release there was the full documentation here. Otherwise, on current master (without my change) the module just vanishes from the rustdoc, without even the deprecation tag being visible, which isn't super helpful to anyone currently using that path and wondering how to upgrade.

The goal is the deprecation message would provide the upgrade hint. The pattern in the past is to hide deprecated APIs from docs.

Irrespective of conventions, my change improves the rustdoc by showing the deprecation and a link. The deprecation note is still useful/visible when compiling something that uses this path. Also I removed the much larger doc-comment section that would show up without the doc(hidden), so that is also at parity.

If you feel strongly that deprecated re-exports must not be documented (I respectfully disagree), then I can put the #[doc(hidden)] back and remove my new doc-comment and link.

Either way, it can be sorted later.

carllerche · 2018-07-05T18:00:45Z

tokio-threadpool/src/lib.rs

-// thread is able to block. Once it finishes processing the blocking future, the
-// thread has no additional work and is inserted into the backup pool. This
-// makes it available to other workers that encounter a `blocking` call.
+//! A work-stealing based thread pool for executing futures.


This documentation was mostly intended for those who are exploring the source. Do you think it is useful to users of the library?

Yes! Most importantly, because this is the only place where the two types of threads (worker and backup) are described. These details are essential to gaining sufficient understanding of the blocking strategy.

Toward making these docs more generally useful, maybe the ¶ Crate layout section could be renamed ¶ Implementation notes and moved to the end?

I would suggest that the three following sections stay as code comments as I don't see them providing any insight to users:

Crate layout

Sleeping workers

Notifying workers

The other sections are probably fine to include in the docs.

One point of potential clarification for backup threads. Is it correct to say that if there is a fully sleeping backup thread at the point where a new worker is needed (blocking called on an existing worker thread), then sleeping backup can be promoted to a new worker? Or is that more a potential future optimization then a current one? It would also be fare if you told me that the rustdoc (-threadpool root) can omit that level of detail.

As I also stated below, I don't think this an essential clarification needed to merge this. Can improve that latter, with a more targetted/ less "general" PR.

carllerche · 2018-07-05T18:02:07Z

src/executor/mod.rs

 pub mod thread_pool {
- //! Maintains a pool of threads across which the set of spawned tasks are


All this doc can probably be moved to tokio-threadpool/src/lib.rs as the crate leevele docs there?

I agree that some (but looking at specifics, not necessarily all) of the original remains useful and could be reused for tokio-threadpool. I will include that in update plan.

dekellum · 2018-07-08T17:03:07Z

I was out of town for your reply, thanks. I'll reply to specific questions and guidance, and conclude by proposing a set of steps to move this forward.

dekellum · 2018-07-08T17:10:58Z

Regarding "two paths" and two use cases, would it be a reasonable goal for the tokio crate, façade rustdocs to become more overview ("how [things] fit together") with a link to the specific crate, where the lower-level details may be found? I won't go any further with this here, in this PR, but I will start making those adjustments for tokio-fs (in or after #454 is merged), if that is agreeable.

Proposed plan to move this PR forward:

Rebase
Reuse useful parts of tokio::executor::thread_pool for tokio-threadpool crate root rustdoc.
Preserve but separate and de-emphasize "Implementation notes" aspects of tokio-threadpool crate root rustdoc.

Sound reasonable?

docs.rs is smart enough to show docs for the latest M.N.P release when M.N is used in the link. For example: https://docs.rs/mio/0.6/mio/struct.Poll.html ..will show mio 0.6.14 and later docs. While using the `M.N.*` (ASTERISK) syntax also works, `M.N` is the more common usage, so standarize a few existing links to that format.

github: cc tokio-rs#421

Some of the previously hidden rustdoc, deleted in: 30e0034 Replace hidden tokio::executor::thread_pool docs with deprecation note ...is here reused in the tokio-threadpool crate root rustdoc.

...and general improvements and linking.

dekellum · 2018-07-08T19:13:56Z

I hope so! Rebased, and added the changes as planned.

dekellum · 2018-07-11T03:56:24Z

I'm hoping for feedback on this with us both back in town? Thanks!

carllerche

Thanks,

I responded to your thoughts. Also, if you could avoid rebasing PRs after an initial review, that would be appreciated. Rebasing makes it harder to tell what exactly changed since last review.

dekellum · 2018-07-11T19:09:33Z

I only rebased because there was a new conflict after the first two weeks, but I will defer to your preference in the future. There is no conflict now, so my (new updated, latest feedback) plan is to hide the three sections you indicate. Sound reasonable?

@carllerche

Per review suggestion by @carllerche on tokio-rs#450, this essentially reverts the promotion of these low-level detail sections, as done originally in: 014229f Promote tokio-threadpool crate level comments to rustdoc github: cc tokio-rs#421

dekellum · 2018-07-11T19:50:16Z

OK, that one point of potential clarification could be adding in another PR, in any case. How is this as of a0880e0 as far as merge acceptability then?

dekellum · 2018-07-11T20:14:42Z

Resolved conflicts by a merge from master, instead of rebase, with ad04b61.

dekellum · 2018-07-11T22:41:11Z

All checks pass @carllerche. Are we good here?

dekellum · 2018-07-13T15:23:36Z

Hey @carllerche I thought we were pretty close to resolving this one. Have I done something to offend or done something wrong with the github review UI?

dekellum · 2018-07-19T22:07:18Z

@carllerche, was there something more I was supposed to do on my end with this? Otherwise, can it be merged? I have runtime docs on my TODO list but at lower priority, I hope you understand, if I can't get this merged.

kpp · 2018-07-20T17:33:44Z

Have I done something to offend

No =) The PR seems good to me but you have to rebase on master to fix conflicts.

dekellum · 2018-07-20T20:26:08Z

OK, merged again.

carllerche · 2018-07-22T20:14:06Z

Sorry for the delay (have a new baby). I'll try to take a look asap.

carllerche

👍

dekellum · 2018-07-23T17:45:57Z

Great, thanks (and congrats!)

dekellum · 2018-08-04T15:22:20Z

Listed as dependency of #437.

kpp mentioned this pull request Jun 25, 2018

cargo doc breaks for 2018-06-18 nightly #437

Closed

dekellum mentioned this pull request Jun 27, 2018

Additional details for tokio-fs rustdoc #454

Merged

carllerche reviewed Jul 5, 2018

View reviewed changes

lnicola mentioned this pull request Jul 6, 2018

tokio-fs: 0.1.2 release #467

Closed

3 tasks

dekellum added 8 commits July 8, 2018 10:48

Fix missing or malformed rustdoc links

f38630b

executor lib rustdoc minor format change

70c6542

Promote tokio-threadpool crate level comments to rustdoc

a0e4190

github: cc tokio-rs#421

Replace hidden tokio::executor::thread_pool docs with deprecation note

30e0034

Fix typo/simplify util module rustdoc

234ba3c

Reuse some tokio::executor::thread_pool rustdoc for the crate

9a6652b

Some of the previously hidden rustdoc, deleted in: 30e0034 Replace hidden tokio::executor::thread_pool docs with deprecation note ...is here reused in the tokio-threadpool crate root rustdoc.

Move thread-pool implementation notes to separate section

311d7cc

...and general improvements and linking.

dekellum force-pushed the general-rustdoc-improvements branch from 58498a8 to 311d7cc Compare July 8, 2018 19:13

carllerche requested changes Jul 11, 2018

View reviewed changes

Hide implementation notes from rustdoc

a0880e0

Per review suggestion by @carllerche on tokio-rs#450, this essentially reverts the promotion of these low-level detail sections, as done originally in: 014229f Promote tokio-threadpool crate level comments to rustdoc github: cc tokio-rs#421

Merge branch 'master' into general-rustdoc-improvements

ad04b61

Merge branch 'master' into general-rustdoc-improvements

250d0c0

carllerche approved these changes Jul 22, 2018

View reviewed changes

carllerche merged commit 491f158 into tokio-rs:master Jul 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General rustdoc improvements #450

General rustdoc improvements #450

dekellum commented Jun 25, 2018

dekellum commented Jun 27, 2018

carllerche commented Jul 5, 2018

carllerche commented Jul 5, 2018

carllerche left a comment

carllerche Jul 5, 2018

dekellum Jul 8, 2018 •

edited

Loading

carllerche Jul 11, 2018

dekellum Jul 11, 2018

carllerche Jul 22, 2018

carllerche Jul 5, 2018

dekellum Jul 8, 2018

carllerche Jul 11, 2018

dekellum Jul 11, 2018

dekellum Jul 11, 2018

carllerche Jul 5, 2018

dekellum Jul 8, 2018

dekellum commented Jul 8, 2018

dekellum commented Jul 8, 2018 •

edited

Loading

dekellum commented Jul 8, 2018

dekellum commented Jul 11, 2018

carllerche left a comment

dekellum commented Jul 11, 2018

dekellum commented Jul 11, 2018

dekellum commented Jul 11, 2018 •

edited

Loading

dekellum commented Jul 11, 2018

dekellum commented Jul 13, 2018

dekellum commented Jul 19, 2018

kpp commented Jul 20, 2018

dekellum commented Jul 20, 2018

carllerche commented Jul 22, 2018

carllerche left a comment

dekellum commented Jul 23, 2018

dekellum commented Aug 4, 2018

		pub mod thread_pool {
		//! Maintains a pool of threads across which the set of spawned tasks are

General rustdoc improvements #450

General rustdoc improvements #450

Conversation

dekellum commented Jun 25, 2018

dekellum commented Jun 27, 2018

carllerche commented Jul 5, 2018

carllerche commented Jul 5, 2018

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekellum Jul 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekellum commented Jul 8, 2018

dekellum commented Jul 8, 2018 • edited Loading

dekellum commented Jul 8, 2018

dekellum commented Jul 11, 2018

carllerche left a comment

Choose a reason for hiding this comment

dekellum commented Jul 11, 2018

dekellum commented Jul 11, 2018

dekellum commented Jul 11, 2018 • edited Loading

dekellum commented Jul 11, 2018

dekellum commented Jul 13, 2018

dekellum commented Jul 19, 2018

kpp commented Jul 20, 2018

dekellum commented Jul 20, 2018

carllerche commented Jul 22, 2018

carllerche left a comment

Choose a reason for hiding this comment

dekellum commented Jul 23, 2018

dekellum commented Aug 4, 2018

dekellum Jul 8, 2018 •

edited

Loading

dekellum commented Jul 8, 2018 •

edited

Loading

dekellum commented Jul 11, 2018 •

edited

Loading