Update docs to conform to Tokio standards #203
Conversation
|
I'd strongly recommend using |
|
@FrankReh that's a good point, maybe this should be narrowed to only doc changes, and API changes can go in another PR. |
I am with you on this. I started off working only on documentation, but I couldn't get past the weird/inconsistent settings for sq and cq entries. I agree that the implementation is a bit weird, it is a side effect of (sometimes) transparently using the As you suggest, I will submit the doc changes as a PR. |
|
It's probably best to leave this open as a PR for the doc changes, due to the feedback already present. |
I typically follow this workflow, however, in this case, it is not terribly convenient. My work computer is a macbook, and my dev box is a corporate managed Linux VM with limited access. I'll see if I can come up with a convenient way to sync the docs from my dev box (I need to create a static webserver for documentation anyways) |
|
This PR would be good to use for the comments. The title or description can be changed to just reference improving comments when it is squash-merged. |
rrichardson
changed the title
There was a problem hiding this comment.
Lots of very good improvements to the comments, thank you.
Sorry, I know my comments about comments sound harsh today. Not having a quiet/let-the-chips-fall-where-they-may-day I guess.
Also, I know the interface for setting the io-uring builder isn't idiomatic. I wasn't aware of any problems with it though. And certainly before ever getting to 1.0, we would probably just copy all the io-uring builder methods and pass them through. I expected the io-uring builder API to still be changing frequently so didn't want to duplicate the efforts yet. Maybe the uring API isn't changing so much anymore these days either.
|
Ok. I've removed the API changes, and also found a few more spots where the docs needed updating. Also, I've created #204 to discuss changes to the Builder API |
|
Hopefully that's everything? |
| /// complete, and yielding its resolved result. Any tasks, futures, or timers | ||
| /// which the future spawns internally will be executed on this runtime. | ||
| /// | ||
| /// Any spawned tasks will be suspended after `block_on` returns. Calling |
There was a problem hiding this comment.
@Noah-Kennedy needs to judge whether this statement about block_on is correct and whether it is correct to imply that block_on could be called more than once. Maybe it can and this is absolutely correct; I'm just saying I'm not sure myself
There was a problem hiding this comment.
It would be good to get a comment, but I based those docs on the tokio docs for block_on
https://docs.rs/tokio/latest/tokio/runtime/struct.Runtime.html#current-thread-scheduler
There was a problem hiding this comment.
Again, if we don't hear from @Noah-Kennedy, you can just remove this change for now because while I like comments, I really care about technical comments that help us understand the tricky stuff when wanting to modify the design. The over all stuff about how to use it and how it relates to tokio I would prefer to leave to Noah over time.
|
On a side note: I am really hoping that pushing a new version of the docs will fix the existing online docs, because they're broke AF |
| /// complete, and yielding its resolved result. Any tasks, futures, or timers | ||
| /// which the future spawns internally will be executed on this runtime. | ||
| /// | ||
| /// Any spawned tasks will be suspended after `block_on` returns. Calling |
There was a problem hiding this comment.
Again, if we don't hear from @Noah-Kennedy, you can just remove this change for now because while I like comments, I really care about technical comments that help us understand the tricky stuff when wanting to modify the design. The over all stuff about how to use it and how it relates to tokio I would prefer to leave to Noah over time.
There was a problem hiding this comment.
Thanks. Will wait 'till the end of the day in case @Noah-Kennedy wants to comment further, otherwise I'll merge what's there.
|
@rrichardson Would it be easy for you to merge from mainline to pick up the change to udp.rs from earlier today and push again? |
|
@rrichardson I will merge this as soon as you update this branch from master and resolve conflicts. |
their necessary invariants. Updated docs to use the indicative mood where applicable.
Co-authored-by: Noah Kennedy <Nomaxx117@gmail.com>
…ing the size of the doc
Done. |
|
Unfortunately the latest comment addition doesn't get past a CI documentation check. |
|
Thanks! |
This PR does
32 things:1. Alters the Runtime crate::Builder API slightly to make it easier to correctly set sq and cq entry counts.2. Updates method documentation to use indicative mood. This is not a huge change, as most public method docs were conformant.
3. Enhances other documentation in a few places for clarity.