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 expression-evaluation exercise: more patterns, more enums #1582

Merged
merged 4 commits into from
Dec 14, 2023

Conversation

djmitche
Copy link
Collaborator

@djmitche djmitche commented Dec 12, 2023

This modifies the exercise to use the standard Result type, based on feedback that students could understand it sufficiently at this point in the course.

Addresses #1565.

// ANCHOR: Expression
/// An expression, in tree form.
/// A numerical expression, in tree form.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's no longer just numerical

@djmitche djmitche enabled auto-merge (squash) December 12, 2023 23:46
@hurryabit
Copy link
Contributor

I think using Result from std is a good idea. I know you're trying to make the expression language more complex to have more pattern matching. Without that, the solution still would have two cases of pattern matching:

  1. Figure out if an expression is a binary operation or just a number literal (or atom).
  2. Figure out the operation used.

IMO, that's enough. Making it more complicated risks confusing the students without gaining any more insights.

@mgeisler
Copy link
Collaborator

IMO, that's enough. Making it more complicated risks confusing the students without gaining any more insights.

For context, Martin is currently teaching the 4-day class here in Zurich, so this seems like valuable feedback to me.

@mgeisler
Copy link
Collaborator

It also uses the standard Result type, based on feedback that students could understand it sufficiently at this point in the course.

Very nice!

@djmitche djmitche merged commit 085cbf2 into google:main Dec 14, 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.

None yet

4 participants