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

Update full code reference main.rs in ch20-03 #3857

Merged
merged 5 commits into from
Apr 4, 2024
Merged

Update full code reference main.rs in ch20-03 #3857

merged 5 commits into from
Apr 4, 2024

Conversation

chris-t-jansen
Copy link
Contributor

Updated the full code reference for main.rs at the end of Chapter 20.3 to match the code the tutorial instructs you to write. Namely:

  • Formats the use statements to match the format in the earlier code blocks
  • Updates the handle_connection function to use BufReader and a match expression per the earlier code blocks

Updated the full code reference for `main.rs` at the end of Chapter 20.3 to match the code the tutorial instructs you to write. Namely:
* Formats the `use` statements to match the format in the earlier code blocks
* Updates the `handle_connection` function to use `BufReader` and a `match` expression per the earlier code blocks
Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

I am overall very much in favor of this change! However, what I think we actually need to do here is:

  • Use the code from listing-20-24 and listing-20-25 as the baseline.
  • Fix the code in listing-20-25 as well. It looks like that predates the switch to using BufReader, and got missed when updating everything else, presumably because it isn’t visible in the text. (Or some other history; I have not dug through in detail!)

Doing that will make sure everything is more in sync. If you’re up for making those changes, please do, and I will see about getting them merged. If not, I can pick it up by pushing to your branch, or by cherry-picking your commits over to a new branch!

@chris-t-jansen
Copy link
Contributor Author

I appreciate the insight and direction! If it's something that needs to happen in a pretty timely fashion, I'm more than happy to turn it over to someone more capable! I'd love to take a swing at it though, as the contribution process is good practice for me.

I think however I might be misunderstanding the changes you're asking for; the code in listing-20-24 and listing-20-25 look to me like they match the full code references at the end of Chapter 20.3, and I don't think this commit alters either of those sections. I apologize for misunderstanding, but I just want to be sure I fully understand the suggestion before I start re-writing the commit! Again, if it'd be easier for someone else to take it on, that's fine too.

@chriskrycho
Copy link
Contributor

No big hurry at the moment, nope, so feel free to chip away at it as makes sense! We are big fans of people being able to tackle things like this as ways to contribute.1

As for the listings, I was slightly unclear. You are correct that listing-20-24 looks like what we want! What I was saying is that we should make no-listing-07-final-code match what is in those two listings, whereas what you did here (perfectly reasonable on its own terms!) has some differences from them, including around the style used for use and so on.

For listing-20-25, though, it has the same issues as you were fixing with this PR:

let mut buffer = [0; 1024];
stream.read(&mut buffer).unwrap();

So we want to update that one to match as well! That would be less evident from the text itself, since it is only presenting a subset of the full code sample.

Let me know if that is still unclear, and sorry about the confusion. 😅

Footnotes

  1. Contributing fixes like this to Rust docs materials—specifically, to Rust by Example—is how I got started in the Rust community, in fact!

@chris-t-jansen
Copy link
Contributor Author

I genuinely had no idea that there was an entire .rs file behind all of those listings. This is truly an object lesson in the importance of familiarizing yourself with the codebase before you start submitting pull requests! I apologize for the confusion, that's my bad!

Follow-up question: should I change the previous listings in the chapter to stylistically match the code in listing-20-24 and listing-20-25 then too (e.g. edit the use statements in listing-20-02)? Specifically, changing the use of the match statement in listing-20-10 onward would require a change in the text of the chapter as well. I'm happy to make that change, I just assumed that the use of match was the more idiomatic way to write it in Rust.

@chriskrycho
Copy link
Contributor

No worries – this is an unusually complicated code base, and I only have a clear understanding of it myself because @carols10cents explained it to me when I came on to work on the 2024 Edition revision and async/await chapter!

Follow-up question: should I change the previous listings in the chapter to stylistically match the code in listing-20-24 and listing-20-25 then too (e.g. edit the use statements in listing-20-02)? Specifically, changing the use of the match statement in listing-20-10 onward would require a change in the text of the chapter as well. I'm happy to make that change, I just assumed that the use of match was the more idiomatic way to write it in Rust.

Hmm – maybe I misread your change before, because I was confused by how 20-25 diverged from 20-24 and earlier!

It looks like what we really need to do is:

  • Update 20-25 to be as small a delta from 20-24 as possible; the only changes required for 20-25 should be switching back over to using the implementation of handle_connection from 20-24, I think?1

  • Update the listing for the final source to match what should now be in the updated 20-25 listing, and which should be the same as 20-24 other than the anchors.

Sorry for the confusion my previous statement caused!

Footnotes

  1. Note that the actual code in 20-24 and 20-25 should actually be identical; it is just what is being highlighted by the anchors that varies. Whether that is the right thing for these code samples, given that theoretically 20-25 is adding the .take() call, is a different question Carol and I will have to sort out! Don’t worry about that part!

chris-t-jansen and others added 2 commits April 3, 2024 17:12
Updated `main.rs` in both `listing-20-25` and `no-listing-07-final-code` based on `listing-20-24`
@chris-t-jansen
Copy link
Contributor Author

chris-t-jansen commented Apr 3, 2024

I think I got it this time! New commit updates main.rs in listing-20-25 based on listing-20-24, and then subsequently updates no-listing-07-final-code based on listing-20-25 (and by extension, listing-20-24 as well).

The only thing that I think is worth mentioning is this line at the end of the current main.rs in no-listing-07-final-code:

stream.flush().unwrap();

It doesn't appear in listing-20-09, which is the last place in the tutorial that the stream.write_all(... line above appears, so I assume it's safe to discard, but I figured it was worth mentioning. Maybe an artifact of a time when write_all didn't flush the output stream?

Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Thanks! I think removing that for consistency makes sense—it looks like the version of the code where that appeared originally had a .write() call, not a .write_all() call. For real robustness you might actually want it, but I think in this case, just matching the earlier code is the right move. Thanks again!

@chriskrycho chriskrycho merged commit 442eda7 into rust-lang:main Apr 4, 2024
2 checks passed
@chris-t-jansen chris-t-jansen deleted the ch20-full-code-update branch April 5, 2024 05:54
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 9, 2024
Update books

## rust-lang/book

23 commits in 19c40bfd2d57641d962f3119a1c343355f1b3c5e..3131aa4642c627a24f523c82566b94a7d920f68c
2024-04-05 22:09:59 UTC to 2024-03-27 18:14:05 UTC

- Correct the description of Listing 5-6 (rust-lang/book#3878)
- Minor text improvements (rust-lang/book#3790)
- Update full code reference main.rs in ch20-03 (rust-lang/book#3857)
- Make Note text in ch. 20 consistent with other notes (rust-lang/book#3876)
- Consistency fix: start note sentence with a capital (rust-lang/book#3652)
- Update README.md (rust-lang/book#3656)
- Update panic! formatting style for Guess example (rust-lang/book#3767)
- Small correction in ch13-01 (rust-lang/book#3780)
- Fix a wording typo in ch07 (rust-lang/book#3694)
- Fix a typo: remove an extra 's' from ch. 18.01 (rust-lang/book#3874)
- Remove redundant words (rust-lang/book#3672)
- Fix rust-lang#3703 (rust-lang/book#3704)
- Update loop to Result: 10 (rust-lang/book#3711)
- Added "--" between run and args on ch12-03-l12 (rust-lang/book#3726)
- Add  maintenance time section in  appendix-07-nightly-rust.md  (rust-lang/book#3859)
- Update the link to the farsi translation repository (rust-lang/book#3839)
- Update README to use --locked for installing mdbook (rust-lang/book#3830)
- Reword "`union`s" to "a `union`" (rust-lang/book#3738)
- Fix two typos in 04-03 and 07-03 (rust-lang/book#3849)
- Fix typo in Chapter 7 Section 3 (rust-lang/book#3743)
- Improve ch03-05-control-flow collection looping wording (rust-lang/book#3758)
- Fix missing column separator (rust-lang/book#3855)
- Update compiler message (rust-lang/book#3856)

## rust-lang/edition-guide

3 commits in 98b33e9a441457b0a491fe1be90e7de64eafc3e5..eb3eb80e106d03250c1fb7c5666b1c8c59672862
2024-03-27 20:44:27 UTC to 2024-03-26 19:26:15 UTC

- some mdbook conveniences (rust-lang/edition-guide#297)
- typo (rust-lang/edition-guide#296)
- Clean up the `editions/index.md` page (rust-lang/edition-guide#294)

## rust-embedded/book

1 commits in 2e95fc2fd31d669947e993aa07ef10dc9828bee7..aa7d4b0b4653ddb47cb1de2036d090ec2ba9dab1
2024-04-05 07:42:54 UTC to 2024-04-05 07:42:54 UTC

- Dependencies: changed "qemu-arch-extra" to "qemu-system-arm" on arch section (rust-embedded/book#368)

## rust-lang/nomicon

2 commits in 6bc2415218d4dd0cb01433d8320f5ccf79c343a1..0d5f88475fe285affa6dbbc806e9e44d730797c0
2024-04-06 13:51:11 UTC to 2024-04-03 02:23:07 UTC

- chore: fix typo (rust-lang/nomicon#448)
- add link to reference about undefined behavior (rust-lang/nomicon#447)

## rust-lang/reference

3 commits in 984b36eca4b9293df04d5ba4eb5c4f77db0f51dc..55694913b1301cc809f9bf4a1ad1b3d6920efbd9
2024-04-03 21:31:14 UTC to 2024-04-01 19:56:13 UTC

- Add the `#[diagnostic]` attribute namespace and the `#[diagnostic::on_unimplemented]` feature to the reference (rust-lang/reference#1449)
- type-layout: be more specific about 32-bit alignments (rust-lang/reference#1393)
- Fix clippy warning in procedural macro example (rust-lang/reference#1488)

## rust-lang/rust-by-example

1 commits in 7601e0c5ad29d5bd3b518700ea63fddfff5915a7..60d34b5fd33db1346f9aabfc0c9d0bda6c8e42be
2024-04-07 13:00:53 UTC to 2024-04-07 13:00:53 UTC

- chore: fix some typos (rust-lang/rust-by-example#1833)

## rust-lang/rustc-dev-guide

11 commits in ffa246b..b77a34b
2024-04-06 20:41:09 UTC to 2024-03-27 08:49:05 UTC

- Explicitly mention compiletest directives are supported in rmake.rs (rust-lang/rustc-dev-guide#1949)
- Add docs for sharded descriptions (rust-lang/rustc-dev-guide#1959)
- Add basic docs for the new `aux-bin` header (rust-lang/rustc-dev-guide#1942)
- Add needs-threads header command (rust-lang/rustc-dev-guide#1943)
- Fix some broken links under bootstrapping. (rust-lang/rustc-dev-guide#1958)
- Replace -Zno-parallel-llvm with -Zno-parallel-backend (rust-lang/rustc-dev-guide#1957)
- Rewrite the `Parameter Environments` chapter (rust-lang/rustc-dev-guide#1953)
- Add quickstart for how to build and run the compiler (rust-lang/rustc-dev-guide#1951)
- Delete length check (rust-lang/rustc-dev-guide#1952)
- Fix some comments (rust-lang/rustc-dev-guide#1950)
- add opaque-types-region-inference-restrictions (rust-lang/rustc-dev-guide#1948)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2024
Rollup merge of rust-lang#123636 - rustbot:docs-update, r=ehuss

Update books

## rust-lang/book

23 commits in 19c40bfd2d57641d962f3119a1c343355f1b3c5e..3131aa4642c627a24f523c82566b94a7d920f68c
2024-04-05 22:09:59 UTC to 2024-03-27 18:14:05 UTC

- Correct the description of Listing 5-6 (rust-lang/book#3878)
- Minor text improvements (rust-lang/book#3790)
- Update full code reference main.rs in ch20-03 (rust-lang/book#3857)
- Make Note text in ch. 20 consistent with other notes (rust-lang/book#3876)
- Consistency fix: start note sentence with a capital (rust-lang/book#3652)
- Update README.md (rust-lang/book#3656)
- Update panic! formatting style for Guess example (rust-lang/book#3767)
- Small correction in ch13-01 (rust-lang/book#3780)
- Fix a wording typo in ch07 (rust-lang/book#3694)
- Fix a typo: remove an extra 's' from ch. 18.01 (rust-lang/book#3874)
- Remove redundant words (rust-lang/book#3672)
- Fix rust-lang#3703 (rust-lang/book#3704)
- Update loop to Result: 10 (rust-lang/book#3711)
- Added "--" between run and args on ch12-03-l12 (rust-lang/book#3726)
- Add  maintenance time section in  appendix-07-nightly-rust.md  (rust-lang/book#3859)
- Update the link to the farsi translation repository (rust-lang/book#3839)
- Update README to use --locked for installing mdbook (rust-lang/book#3830)
- Reword "`union`s" to "a `union`" (rust-lang/book#3738)
- Fix two typos in 04-03 and 07-03 (rust-lang/book#3849)
- Fix typo in Chapter 7 Section 3 (rust-lang/book#3743)
- Improve ch03-05-control-flow collection looping wording (rust-lang/book#3758)
- Fix missing column separator (rust-lang/book#3855)
- Update compiler message (rust-lang/book#3856)

## rust-lang/edition-guide

3 commits in 98b33e9a441457b0a491fe1be90e7de64eafc3e5..eb3eb80e106d03250c1fb7c5666b1c8c59672862
2024-03-27 20:44:27 UTC to 2024-03-26 19:26:15 UTC

- some mdbook conveniences (rust-lang/edition-guide#297)
- typo (rust-lang/edition-guide#296)
- Clean up the `editions/index.md` page (rust-lang/edition-guide#294)

## rust-embedded/book

1 commits in 2e95fc2fd31d669947e993aa07ef10dc9828bee7..aa7d4b0b4653ddb47cb1de2036d090ec2ba9dab1
2024-04-05 07:42:54 UTC to 2024-04-05 07:42:54 UTC

- Dependencies: changed "qemu-arch-extra" to "qemu-system-arm" on arch section (rust-embedded/book#368)

## rust-lang/nomicon

2 commits in 6bc2415218d4dd0cb01433d8320f5ccf79c343a1..0d5f88475fe285affa6dbbc806e9e44d730797c0
2024-04-06 13:51:11 UTC to 2024-04-03 02:23:07 UTC

- chore: fix typo (rust-lang/nomicon#448)
- add link to reference about undefined behavior (rust-lang/nomicon#447)

## rust-lang/reference

3 commits in 984b36eca4b9293df04d5ba4eb5c4f77db0f51dc..55694913b1301cc809f9bf4a1ad1b3d6920efbd9
2024-04-03 21:31:14 UTC to 2024-04-01 19:56:13 UTC

- Add the `#[diagnostic]` attribute namespace and the `#[diagnostic::on_unimplemented]` feature to the reference (rust-lang/reference#1449)
- type-layout: be more specific about 32-bit alignments (rust-lang/reference#1393)
- Fix clippy warning in procedural macro example (rust-lang/reference#1488)

## rust-lang/rust-by-example

1 commits in 7601e0c5ad29d5bd3b518700ea63fddfff5915a7..60d34b5fd33db1346f9aabfc0c9d0bda6c8e42be
2024-04-07 13:00:53 UTC to 2024-04-07 13:00:53 UTC

- chore: fix some typos (rust-lang/rust-by-example#1833)

## rust-lang/rustc-dev-guide

11 commits in ffa246b..b77a34b
2024-04-06 20:41:09 UTC to 2024-03-27 08:49:05 UTC

- Explicitly mention compiletest directives are supported in rmake.rs (rust-lang/rustc-dev-guide#1949)
- Add docs for sharded descriptions (rust-lang/rustc-dev-guide#1959)
- Add basic docs for the new `aux-bin` header (rust-lang/rustc-dev-guide#1942)
- Add needs-threads header command (rust-lang/rustc-dev-guide#1943)
- Fix some broken links under bootstrapping. (rust-lang/rustc-dev-guide#1958)
- Replace -Zno-parallel-llvm with -Zno-parallel-backend (rust-lang/rustc-dev-guide#1957)
- Rewrite the `Parameter Environments` chapter (rust-lang/rustc-dev-guide#1953)
- Add quickstart for how to build and run the compiler (rust-lang/rustc-dev-guide#1951)
- Delete length check (rust-lang/rustc-dev-guide#1952)
- Fix some comments (rust-lang/rustc-dev-guide#1950)
- add opaque-types-region-inference-restrictions (rust-lang/rustc-dev-guide#1948)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chapter 20. Listing 20-25 is inconsistent with the rest of the code in the chapter
2 participants