-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Split let control flow into mutliple sub-slides #2567
Conversation
There are three kinds of syntax here, making for a very long and hard-to-navigate slide. Splitting it up helps!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a good idea.
if let Some(digit) = first_byte_char.to_digit(16) { | ||
Ok(digit) | ||
} else { | ||
return Err(String::from("not a hex digit")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're at this: Is the return
here very idiomatic? It's basically in tail position, so just having Err(...)
would seem better to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I agree this was not idiomatic, the code on this section is intended to be rewritten to use let-else. Therefore it might actually be desirable to keep the returns so that presenters can just copy-paste the returns into the let-else rather than having to add them manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's why they existed. @hurryabit what are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, there's more value if the attendees get to compare two idiomatic versions. In fact, having to write a little more slows down things a bit and gives people more time to digest what we're teaching them. I would think that the slowdown introduced by having to write return
three times is very reasonable.
All that said, I'd prefer to start with the idiomatic version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's what the current version of the PR is -- do you want to mark it approved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I already had approved. Here we go.
danke! |
There are three kinds of syntax here, making for a very long and hard-to-navigate slide. Splitting it up helps!