-
Notifications
You must be signed in to change notification settings - Fork 1.8k
add typestate pattern chapter for idiomatic rust #2821
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
base: main
Are you sure you want to change the base?
Conversation
src/idiomatic/leveraging-the-type-system/typestate-pattern/typestate-generics.md
Show resolved
Hide resolved
src/idiomatic/leveraging-the-type-system/typestate-pattern/typestate-generics.md
Outdated
Show resolved
Hide resolved
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.
As @gribozavr suggested, I think both of these slides would benefit from being split in two, where we start with a suboptimal API that allows incorrect usage, then demonstrate how typestate can make incorrect usage impossible.
src/idiomatic/leveraging-the-type-system/typestate-pattern/typestate-generics.md
Outdated
Show resolved
Hide resolved
141e680
to
602ef85
Compare
this is again in the flow of a problem statement first, building on our original example, and in next slide we'll add the solution with generics
d727cd7
to
b61c337
Compare
The four slides in the typestate pattern chapter are ready for review again. Note that the generics slide currently exceeds the height limit and fails the corresponding test. I’ve left it as-is for now, since I wasn’t sure how you’d prefer to address that. Let me know how you’d like to proceed. |
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.
This is coming together nicely! I think we still need to break up the generics example since it's too much for a single slide, but the example itself is a good demonstration of typestate.
impl<S> Serializer<S> { | ||
fn buffer_size(&self) -> usize { | ||
// [...] | ||
# self.buffer.len() | ||
} | ||
} |
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 don't think we need buffer_size
, I don't think printing out the buffer size as part of the demo is necessary and having it here adds a bit of noise to an already large example.
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.
The goal was mostly to have at least 1 method that is use-able at any stage of the Serializer
. Not something you deem useful? Or if you do but would like it to be shown differently, how?
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 don't think that needs to be demonstrated directly. The example already shows how we can keep common state in Serializer
, it's not a large leap to figure out this applies to methods as well. I think all we need here is a speaker note calling out that common state/behavior can go in Serializer
.
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 like the example here, I think it does a good job demonstrating a more complex case using typestate, but it is definitely a lot to fit into one slide. I think the move here is to break this up into smaller slides, where each slide focuses on just a portion of the structure, e.g. one for impl<S> Serializer<Struct<S>>
and one for impl<S> Serializer<List<S>>
, etc. Then we'd have a final file that shows how the API is used, along with demonstrating how it rejects incorrect usage. We may not even need to have a slide for every piece of this, maybe we just demonstrate a couple of the cases (e.g. structs and lists), and leave the rest of the setup in the playground.
What you may want to do is put this example in its own .rs
file and then use anchors and imports to pull snippets of it into slides. This is the approach we use in a lot of the exercises.
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.
Something else worth mentioning about this example is that all of the state structs are zero-sized, meaning there's no memory or runtime overhead from constructing complex state types.
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.
What you may want to do is put this example in its own .rs file and then use anchors and imports to pull snippets of it into slides. This is the approach we use in a lot of the exercises.
Will investigate
Something else worth mentioning about this example is that all of the state structs are zero-sized, meaning there's no memory or runtime overhead from constructing complex state types.
Fair, will see where that fits. Just need to make clear that it no longer would be a ZST
as soon as you do add state in one of them (e.g. for avoiding duplicates in a Struct
)
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.
Just need to make clear that it no longer would be a
ZST
as soon as you do add state in one of them (e.g. for avoiding duplicates in aStruct
)
Ah, true. Maybe just mention that there's no additional overhead from composing these state marker types, and that ones that are ZSTs have no overhead at all. This can just be a speaker note, to be clear.
src/idiomatic/leveraging-the-type-system/typestate-pattern/typestate-generics.md
Show resolved
Hide resolved
```bob | ||
serialize struct start | ||
-+--------------------- | ||
| | ||
+--> serialize struct field | ||
-+--------------------- | ||
| | ||
+--> serialize struct field | ||
-+--------------------- | ||
| | ||
+--> serialize struct end | ||
``` |
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 don't think we need this diagram. The intended flow here is pretty straightforward since the methods are intended to be used in the order they're declared. If you think it's worth explicitly calling out the intended flow then I think just writing it in a note would be enough.
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.
Sure, will handle that. Thank you for pointing it out.
```bob | ||
+------------+ serialize struct +-----------------+ | ||
| Serializer | ------------------> | SerializeStruct | <------+ | ||
+------------+ +-----------------+ | | ||
| | ||
| ^ | | | | ||
| | finish struct | | serialize field | | ||
| +-----------------------------+ +------------------+ | ||
| | ||
+---> finish | ||
``` |
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 this diagram would be useful to show to students, so I think we should move it into the slide.
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.
In that case it will however no longer fit in the screen I think, when combined with the code. That is not an issue?
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 it's fine in this case. We do have a few slides that are a bit longer because they have some diagrams in them, e.g. the Move Semantics slide, and I've found those reasonable to cover in class.
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 for this slide, rather than showing a bunch of stubbed out code, it might be better to show an example of the more complex serialized output that we want to support. That would then lead nicely into a few slides showing how we can build the more complex serializer.
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.
Sure thing, your other comments seem to allude to how to do that.
Gonna think about it a bit more and get back to you with a draft on it next time I get back to this PR. Thank you for all the feedback!
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.
Sounds good! Also, feel free to reach out if you want to further discuss any of my suggestions, I'm happy to discuss and brainstorm further.
I've removed myself as reviewer as a form of load-shedding, but I looked through this a week or so ago and had no objections, so consider this a soft approval :) |
No description provided.