search

Argument-Clinic / cpython

The Python programming language
https://www.python.org/
Other
1 stars 0 forks source link

DSLParser: pass `function` as a parameter to all `state_foo` methods #13

Closed AlexWaygood closed 1 year ago

AlexWaygood commented 1 year ago

Towards #7

AlexWaygood commented 1 year ago

It looks like a big diff, but a lot of it is just me refusing to keep to the existing ~120-character line lengths for bits that I have to touch anyway with this refactor 😅

This doesn't get rid of all the assert isinstance(self.function, Function) assertions. But it does get rid of some. And, more importantly, it means that function is now always passed as a parameter to all methods on the DSLParser. This feels much easier to understand to me than the current situation, where some methods on DSLParser access it via self.function, and others have it passed as a parameter.

Ultimately, if it's sometimes None and sometimes an instance of Function, it just doesn't feels suitable to keep it as state on DSLParser instances.

AlexWaygood commented 1 year ago

Note: I haven't checked test coverage for all the lines I'm touching in this PR. Will do that before filing a PR to CPython -- if you like the overall idea here!

erlend-aasland commented 1 year ago

I'll have a look later! Are you thinking of ripping out the state machine in the long term?

AlexWaygood commented 1 year ago

Are you thinking of ripping out the state machine in the long term?

Not sure... I do feel like the current way of calling all the state_foo methods via self.next() is a bit of a leaky abstraction (the state_foo methods just have too many differences between them!), and this problem is sort-of a symptom of that. But tearing out the state machine entirely would be quite the task...

Still, whatever the case, this change definitely makes me feel happier about the overall design. So I think it's a good step for now :)

AlexWaygood commented 1 year ago

Actually, I'll just close this in favour of #14. I was expecting that to be a bigger diff than it was, but it's only a slightly bigger change than this one (and it's built on top of this one). It gets us to a much nicer place imo.