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

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

Closed
wants to merge 1 commit into from

Conversation

AlexWaygood
Copy link

Towards #7

@AlexWaygood
Copy link
Author

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.

Comment on lines +4700 to +4701
assert function is None

Copy link
Author

Choose a reason for hiding this comment

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

This assertion (and a few others I add in this PR) isn't actually necessary to keep mypy happy, but I think it's useful to assert exactly where we are in the state machine at this point. I.e., it's there as a sanity check and for readability.

@AlexWaygood AlexWaygood changed the title DSLParser: pass function as a parameter DSLParser: pass function as a parameter to all state_foo methods Aug 3, 2023
@AlexWaygood
Copy link
Author

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!

return function

def state_dsl_start(self, function: Function | None, line: str) -> Function | None:
assert function is None
Copy link
Author

Choose a reason for hiding this comment

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

(Another assertion which isn't required for mypy, but which I added because I thought it provided clarity for human readers about where we are in the state machine)

@erlend-aasland
Copy link

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

@AlexWaygood
Copy link
Author

AlexWaygood commented Aug 3, 2023

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
Copy link
Author

AlexWaygood commented Aug 3, 2023

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.

@AlexWaygood AlexWaygood closed this Aug 3, 2023
@AlexWaygood AlexWaygood deleted the self-dot-function branch August 3, 2023 18:10
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