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

Rework the error handling exercise to be based on the expression evaluator exercise #2521

Merged
merged 4 commits into from
Dec 16, 2024

Conversation

randomPoison
Copy link
Collaborator

This PR follows up from #2517, taking the error handling that was removed in that PR and reintroducing it as the error handling exercise. The core goal of the exercise is the same as before: Take some code that panics and make it return a Result, but I think this change is an improvement in a few ways:

  • The new exercise is simpler, focusing on a single function with a single error case.
  • It still requires that students both create an error and propagate it with ?.
  • It removes the need for thiserror as an external crate, simplifying the solution and allowing it to be tested with mdbook test.
  • It demonstrates that an error type can be as simple as an empty struct.
  • Reusing the solution from a previous exercise should reduce the upfront cognitive burden for students, hopefully meaning they can focus more on solving the solution instead of trying to wrap their head around a parser implementation.

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.

This looks great, maybe just update the comments as noted.

Operation::Add => left + right,
Operation::Sub => left - right,
Operation::Mul => left * right,
Operation::Div => if right != 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

src/pattern-matching/exercise.rs does not contain this conditional (it just relies on / to panic).

I think that's OK, but maybe claiming it's "the original" in the comment above is inaccurate enough to bother folks (like me)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah true it's not exactly the same as the solution to the earlier exercise, I deliberately added in the panic to show students where exactly the error case is. Personally I don't think that'd be too confusing for students, but let me add a speaker note calling that out to teachers so they can mention it if students are confused.

@randomPoison randomPoison merged commit 4663ec8 into main Dec 16, 2024
37 checks passed
@randomPoison randomPoison deleted the legare/rework-error-handling-exercise branch December 16, 2024 22:36
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