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

Various small fixes #1556

Merged
merged 1 commit into from
Dec 5, 2023
Merged

Various small fixes #1556

merged 1 commit into from
Dec 5, 2023

Conversation

marshallpierce
Copy link
Collaborator

Plus one more substantial comment on casting.

@@ -62,4 +62,4 @@ Notes on stack returns:

</details>

[Playground]: https://play.rust-lang.org/
[Playground]: https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=0cb13be1c05d7e3446686ad9947c4671
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

playground share link for release mode on the above code

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just in case you haven't thought of it: we don't have a good way to maintaining these links as we modify the code. So I've been resisting adding them so far.

But let's see how it goes: perhaps we won't ever modify this and then the link is great 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was the only place that I found the playground useful, which is why I bothered... but yes, the maintenance concern does make me uneasy. If there was a safe way to avoid spamming playground with these snippets (content addressable storage style, perhaps) at each build, maybe we could automatically populate these links.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we could automatically populate these links.

It's possible to embed the code directly into the link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&code=fn+main%28%29+%7B%0A++++let+x+%3D+42%3B%0A%7D

So one could in principle (assuming the server accepts very long URLs) make a mdbook plugin which constructs such URLs for us automatically.

src/memory-management/exercise.rs Show resolved Hide resolved
src/methods-and-traits/methods.md Show resolved Hide resolved
src/slices-and-lifetimes/lifetime-elision.md Show resolved Hide resolved
src/slices-and-lifetimes/slices.md Show resolved Hide resolved
src/std-traits/default.md Show resolved Hide resolved
src/std-traits/from-and-into.md Show resolved Hide resolved
src/user-defined-types/enums.md Show resolved Hide resolved
src/user-defined-types/named-structs.md Show resolved Hide resolved

For infallible casts (e.g. `u32` to `u64`), prefer using `From` or `Into` over `as`
to confirm that the cast is in fact infallible. For fallible casts, `TryFrom` and `TryInto` are available when you want to handle casts that fit differently from those that don't.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was the one bit of content that seemed to be missing detail to me, so I fleshed it out with guidance derived from what my team has found to work well doing a fair bit of bit wrangling.

Side note: I expected dprint fmt to wrap this, but it didn't 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice work!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Side note: I expected dprint fmt to wrap this, but it didn't 🤷

This is because we've been excluding the src/ directory until now. I have #1157 up for this, but I was waiting for the v2 rewrite to land first. I will re-fresh that PR now and then we can have everything consistent.

@marshallpierce marshallpierce marked this pull request as ready for review December 5, 2023 19:06
Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

Great fixes - thank you!

@@ -36,7 +36,7 @@ fn main() {
For the common case of matching a pattern and returning from the function, use
[`let
else`](https://doc.rust-lang.org/rust-by-example/flow_control/let_else.html).
The "else" case must diverge (`return`, `break`, or panic - anything but
The "else" case must evaluate to the same type as the `if` block, or diverge (`return`, `break`, or panic - anything but
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is true for let else.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, right, except this one :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I mangled up the if let and let else sections; I'll revert this. Maybe let else should get its own section header?

The days are fast paced and we cover a lot of ground!

{{%course outline Fundamentals}}

## Deep Dives

In addition to the 3-day class on Rust Fundamentals, we cover some more
In addition to the 4-day class on Rust Fundamentals, we cover some more
Copy link
Collaborator

Choose a reason for hiding this comment

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

I missed a lot of spots for 3 -> 4!

src/slices-and-lifetimes/lifetime-elision.md Show resolved Hide resolved

For infallible casts (e.g. `u32` to `u64`), prefer using `From` or `Into` over `as`
to confirm that the cast is in fact infallible. For fallible casts, `TryFrom` and `TryInto` are available when you want to handle casts that fit differently from those that don't.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice work!

Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

(for the let-else comment)

Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

Sorry, I'm struggling with the GH UI today :)

Plus one more substantial comment on casting.
@@ -33,6 +33,7 @@ fn main() {
}
```

# `let else` expressions
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Matches # if let expressions above -- maybe overkill?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, makes sense. I didn't realize that was missing!

@@ -33,6 +33,7 @@ fn main() {
}
```

# `let else` expressions
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, makes sense. I didn't realize that was missing!

@djmitche djmitche merged commit 6c5061b into google:main Dec 5, 2023
31 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.

3 participants