-
Notifications
You must be signed in to change notification settings - Fork 77
consider using automat for your state machine? #1
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
Comments
Oops. I realize that automat's README doesn't include any of its generated diagrams. Here's one that automat generated for a TCP state machine: twisted/vertex#28 (comment) |
Hey @glyph, thanks for taking a look :-) I actually considered using automat, but it seemed simpler to write things directly -- I'm not sure if this was because I just ran out of energy to decode the docs (learning things is hard), or because h11's state machine is genuinely a bad fit for automat. Here's the complete state machine code if you're curious, nicely segregated out into its own self-contained file: https://github.com/njsmith/h11/blob/master/h11/state.py Possibly there is a better way to do this, but the state machine I ended up with is a rather weird beast -- it's really two linked state machines, that have a combination of transitions triggered by simple events, by the joint state of the two machines, and by several special-case conditions that are open-coded directly into the logic. Also, the API neither is nor should be driven by method calls corresponding to different events, since I don't use that pattern anywhere in h11. (The way you send a Request is But, you have approximately 3000% more experience implementing network protocols than I do, and probably at least 1000% more experience with HTTP specifically (I, uh, wasn't sure what the request and response lines looked like until I started reading RFC 7230 last week), so it is very possible that I am making some silly mistakes and the current state machine could be improved, and its API/implementation ditto. |
@njsmith - It looks like you've considered this in some details, so at this point it would probably be more tedious to deal with each thing point-by-point than for me to just send you an (unfortunately quite large) PR. I likely won't have time for a few weeks, but would it be worth my time to do so? If the code made sense to you and didn't have the issues you perceive as potentially problematic about automat, would you land it? It would probably be easier to consider things that way. |
Not sure how to respond really... I'm certainly interested in things that make code simpler / smaller / faster, so if you submit a PR that does some combination of those that then that will be awesome, and i not... not :-). My objections above aren't objections per se, just a lack of me seeing how to use automat to make that happen. I'm not like terrified of external dependencies or anything. Happy to meet up locally (you're in SF, right?) or at PyCon to chat in case that would help too. I did just learn from wikipedia what a mealy machine is (the same as what I think of as a FST, right?), and it isn't immediately obvious to me how to fit the h11 state machines into that framework. |
I am not asking for a guarantee that you'd accept such a PR (that would be silly); I just want to be sure that you'd consider it before I took the trouble of writing it.
Yep, I'm in SF. Perhaps this would be easiest :). |
Going to mark this closed this for now, since from discussion it sounds like the next action item is for @glyph to either make an attempt at implementing this or not, so no need to keep it in on open issues list :-). Please re-open (or open something new or whatever) if/when appropriate. |
Your state diagrams suggest that your state machine is a pure mealy machine. Automatically generated state diagrams are cool, but delegating things to existing libraries is even cooler. Would you consider giving https://github.com/glyph/automat a try? If you decide not to use it, it would be a huge favor if you could file some bugs and give feedback as to why.
The text was updated successfully, but these errors were encountered: