-
Notifications
You must be signed in to change notification settings - Fork 165
dot_parser: Define parser elements in a class #464
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
Conversation
Instead of a global variable `graphparser` that's set by calling a function the first time it's referenced, set up the parser definition in a class `GraphParser`, so that it can be accessed as `GraphParser.parser`. This has the advantage that all of its _component_ parts (like the definition of `subgraph_stmt`, or `attr_list` can also be accessed as `GraphParser.subgraph_stmt` or `GraphParser.attr_list`, etc. (Much better for debugging/testing.) In addition, to improve dependency relationships: - Only import pydot.core into dot_parser, not `pydot` - Change all `pydot.Foo` references to `pydot.core.Foo` - Use concrete `pydot.core.Foo`s in type annotations (instead of strings)
It annoyed mypy in strict mode.
Strictly for compatibility, in case anyone is using it directly. They probably shouldn't be, but this way we won't break their code.
804e0be
to
ce3c952
Compare
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
So, I just added another commit that moves the lower-end RationaleThe code as it currently stands passes MyPy checks on Python 3.9 - 3.13, it only upsets MyPy on Python 3.8 because PyParsing is untyped on that version, and always will be as it's no longer supported by newer PyParsing. And since Python 3.8 is also EOL, while we may still support it, I don't think we should write our code for that version. If the code works on Python 3.8 (and it does), and if it passes type checks on supported Python versions (which it does), then I think that's the highest level of Python 3.8 support we can be expected to provide. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I read it twice and didn't see anything to complain about.
It passes the tests, so...
Now I just have to take advantage of this newfound power and actually figure out why And also possibly hack in support for the undocumented syntax I found last week (#465). |
This PR is centered around one major, should-be-api/user-transparent change to the
dot_parser
code which will change everything for us:The final
ParserElement
that we use for parsing, as well as all of the component parsing fragments used to build it, are now defined in the top level of a class definition namedGraphParser
.The class is not meant to be instantiated (though it can be), and it consists only of class-member variables, all of which are the
ParserElement
components of our parser definition. The final element,GraphParser.parser
, is what used to be stored in the globalgraphparser
symbol. (Which is preserved for compatibility, even though our own code won't use it anymore.)Having access to the parser components is a necessary step towards being able to finally debug, test, and modify the parser. With the new class, I can do things like this:
So now I at least have a starting point to look into the code. It's also possible to disable parse actions (just call
.set_parse_action()
with no arguments on any of theParserElement
objects that have one, to clear it out) in order to get a look at the values being sent into the parse action.And we can write unit tests for all of the parser pieces, now, instead of only testing it as a black box.
Other changes made
dot_parser
now only imports, and only refers to,pydot.core.*
classes; the compatibility exports at thepydot
level are never used. That makes the dependency graph more manageable.pydot.dot_parser
is gone fromcore.py
. It's now only imported on demand, whengraph_from_dot_data()
needs it.dot_parser
are now real references to imported classes, it no longer uses string annotations.s
andloc
arguments are removed from parse action functions that don't use them (something PyParsing has supported for a long while, now).The no-opOK, correction,__init__
is removed from theHTML
class, since its unused type-ignore comment was annoying MyPy and it served no purpose anyway.HTML.__init__()
can't be removed, just its# type: ignore
comment.Notes
This will cause huge conflicts with my other open PR, but it's worth the disruption. I'll do the work of resolving those once they occur.