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

Async chapter edits #3983

Merged
merged 39 commits into from
Aug 5, 2024
Merged

Async chapter edits #3983

merged 39 commits into from
Aug 5, 2024

Conversation

chriskrycho
Copy link
Contributor

@chriskrycho chriskrycho commented Jul 17, 2024

Important

This is the home of our editing process. If you’re someone we asked to review the new async chapter, please see #3909 instead!

  • Update the CI config to include the trpl crate and its dependencies when running mdbook test so it can actually resolve them.
  • Update listings which use the trpl crate to pass CI, primarily by adding extern crate trpl; to each of them. Fix any bugs found by mdbook test.
  • Get the chapter passing the lint check.
  • Integrate review feedback from [WIP] async and .await chapter #3909. → partial, ongoing!

A number of examples *already* do not work correctly, and the inbound
async chapter requires Rust 2021.
- `tests` -> `src/tests/mod.rs`
- `tests::config` -> `src/tests/config.rs`

This is just profoundly easier to work with.
When I originally built this, I thought *all* “listings” had numbers and
captions, but it turns out that there are a number of places in the book
where having the overall `figure`-driven output, i.e. with a file name,
is desirable even though there is no number or caption.

A potential enhancement later would be to require a caption if a number
is present, since that seems to be what the book actually does.
XML does not allow more XML to appear in the body of an attribute, but
this is not XML! It is *HTML*, since Markdown allows embedding HTML, and
HTML *does* allow embedding further `<` and `>` characters within the
attributes on the element. Accordingly, switch to `html_parser`, add a
test covering this behavior, and update `ListingBuilder` to take the
number, caption, and file name types as owned rather than as references,
since that is what `html_parser` supplies.

Additionally, refactor the guts a bit so it is easy to see the overall
logic of `rewrite_listing`, with the gnarly bits around opening and
closing the rewritten listings pushed into a method on the `State`
struct, itself renamed to `ListingState` and its `current_listing` field
renamed to `current`. This also clarifies the semantics of each part of
the rewrite operation, e.g. `ListingState::open_listing` is fallible;
`ListingState::close_listing` is not.
…-support

Improve handling of `<Listing>`s
mdBook does not currently have particularly good support for “external”
crates. To make the test suite work correctly with `trpl`, we must first
build `trpl` itself (`mdbook` will not do it), and then explicitly pass
its `deps` path as a library search path for `mdbook test`. That will
make sure all the crates can be resolved when running the tests.


.github/workflows/main.yml
- Update the contents of the code in the chapter so it is correct, and
  update the text to match. Given this is about `Future`, this may also
  warrant moving/simplifying that whole chunk of code, too.

- Update the listing to use `extern crate` since it does not otherwise
  work correctly with `mdbook test`. Alas.
- Update all the listings in the chapter to use `extern crate` since
  `mdbook test` does not understand Rust 2018. Alas.
- Ignore two of the listings because they never stop.
- Update all the listings in the chapter to use `extern crate` since
  `mdbook test` does not understand Rust 2018. Alas.
- Fix one listing which had gotten out of sync somewhere along the way.
  (This also fixes a comment flagged up by a reviewer!)
- Update all the listings in the chapter to use `extern crate` since
 `mdbook test` does not understand Rust 2018. Alas.
- Ignore a listing which has a missing body apurpose.
- Rewrite the `StreamExt` definition to be more correct, and extract it
  to a standalone “no-listing listing” so we can make sure its
  definition at least type checks.
- Update all the listings in the chapter to use `extern crate` since
 `mdbook test` does not understand Rust 2018. Alas.
- Ignore the listings (inline or otherwise!) which are *intended* not to
  compile, so mdbook does not try to test them.
- Clarify some of the text around the *actual* Tokio definition of the
  `StreamExt::next` method.
This does not change the actual behavior of `mdbook test`, but it does
get rid of a warning about the test renderer being unsupported!
…nderer

infra: support test renderer in mdbook preprocessors
Note: this does *not* include all fixes for the text, only for the links
themselves. For the text, we will also need to search for references to
chapters 17-20. This catches a few of those along the way, but there are
no doubt others.
@chriskrycho
Copy link
Contributor Author

Heyyyy, CI is ✅ at last! That’s a great foundation to keep building on as we do further edits.

Co-authored-by: Tim McNamara <[email protected]>
Co-authored-by: James Munns <[email protected]>
chriskrycho and others added 5 commits July 23, 2024 17:28
Co-authored-by: Carol (Nichols || Goulding) <[email protected]>
Co-authored-by: Will Crichton <[email protected]>
Co-authored-by: Tim McNamara <[email protected]>
Use the original transitional paragraph and structure, adding to it
instead of rewriting it entirely.

HT @timClicks (Tim McNamara <[email protected]>) for pointing
out how my rephrasing here made it worse!
- Drop the history lesson and comparisons to other approaches. Focus on
  what async gives us instead.
- Simplify and clarify the 
- Talk about “the network” instead of “network sockets” and simplify the
  example code to match.
- Swap the ordering to match the order in the text (concurrent is first,
  at least for now).
- Make them go left-to-right instead of top-to-bottom for compactness in
  the text.
- Fix rendering issues resulting from the VS Code extension doing things
  that normal `dot` does not, for who knows what reasons.
ehuss and others added 7 commits July 29, 2024 11:32
Block *everything*. We do not want any of this to be indexed, because we
*only* use it for previews, and do not want its content to be cached or
(especially!) surfaced to readers, since it may have a variety of issues
ranging from pedagogical missteps to outright errors!
infra: add robots.txt for GH Pages previews
infra: include ghp-import and git push in generate-preview script
mdbook-trpl-listing: Add missing elided lifetimes
@chriskrycho chriskrycho merged commit f6d2e30 into only-new-async Aug 5, 2024
6 checks passed
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.

2 participants