search

Argument-Clinic / cpython

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

Move parameter parsing into a dedicated class #17

Open AlexWaygood opened 1 year ago

AlexWaygood commented 1 year ago

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:

https://github.com/Argument-Clinic/cpython/blob/400835ea1626c8c6dcd967c7eabe0dad4a923182/Tools/clinic/clinic.py#L4907-L4917

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 commented 1 year ago

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 commented 1 year ago

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

AlexWaygood commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.