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

Move parameter parsing into a dedicated class #17

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AlexWaygood
Copy link

@AlexWaygood AlexWaygood commented Aug 4, 2023

This is a pretty "dumb" refactor -- I basically just copied and pasted all the methods called here (and all methods that are only ever called by those methods) from the DSLParser into a new ParameterParser class:

cpython/Tools/clinic/clinic.py

Lines 4907 to 4917 in 400835e

match line.lstrip():
case '*':
self.parse_star(func)
case '[':
self.parse_opening_square_bracket(func)
case ']':
self.parse_closing_square_bracket(func)
case '/':
self.parse_slash(func)
case param:
self.parse_parameter(param)

Conceptually, it's much nicer to have these methods in a dedicated namespace, since they're all working on a discrete activity to the rest of the DSLParser. (If we want to do this, it would also free up the verb parse_ for the methods being refactored in #14!)

The diff is pretty unreadable, though ://

@erlend-aasland
Copy link

Hm, the parameter parsing also has a state machine, though implemented differently from the DSL parser state machine. Could we create a small state machine library that they could both use? Currently, we need to understand the peculiarities of both state machines; if they both used the same library, it would greatly improve maintainability.

@AlexWaygood
Copy link
Author

This diff wasn't big enough for you, huh? ;)

@AlexWaygood
Copy link
Author

AlexWaygood commented Aug 4, 2023

In seriousness, though: there was a question in my mind about whether we'd want to do something like this first or something like #14 first. Your answer seems to indicate we should probably do #14 first. This refactor could get quite involved if we try to abstract the two state machines into a common library, which would make #14 the lower-hanging fruit. (It also brings the two state machines somewhat closer together in their implementation, which should make it easier to create a common abstraction for the two state machines.)

@erlend-aasland
Copy link

Yeah, it would be interesting to try to unite the state machines. Let's keep it here, for now (including #14); these changes are pretty invasive, so we'd better get it right before pushing it upstream :)

@erlend-aasland
Copy link

The more I think about these two state machines, the more I think they should be unified to one grand parser state machine. We'd have separate states for the various parameter types, and also separate parameter parsing functions. I'll try to explore this path.

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