Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
new sx structure parser that tolerates unexpected items #1253
base: main
Are you sure you want to change the base?
new sx structure parser that tolerates unexpected items #1253
Changes from 1 commit
3ad3bd1
9ff02a2
f7a1358
d7244bf
ba083f9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Expecting sub classes and base classes to have different signatures for their
__init__
is imo problematic because it violates the substitution principle. If sub classes anyway should callparse
in their init, can the base class init not be something likethen subclasses can just to
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.
I thought about your suggestion, but I am not convinced. If calling the base class initializer means to do the parsing, the base class initializer must be called at the end of the derived class's init . This is contrary to what most people do elsewhere.
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.
Fair, but then I would also keep it out of the sub classes
__init__
. An alternative would be to have the actual parsers not be sub classes of the keyword tree parser at all, but to implement a has-a relationship instead.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.
I'm very uneasy with defining this kind of stuff outside of
__init__
, basically because if this class is used without callingparse
, people would only seeAttributeError
.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.
I think the whole
while
could be replaced byThere 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.
No, because sometimes I need multiple lines at the same time. More importantly, the reading of additional lines can happen elsewhere than in the main parser loop.
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.
Anyone who implements a parser with such an engine and never calls "parse" didn't get the idea of the engine. Initializing in the parse function would allow to parse multiple times with the same object (if this makes sense...).
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.
Ah, I missed the
read_until
parts. Well, I'd want at least the with statement, because currently I don't see the file ever being closed at all.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.
filehandle was implicitly closed by python upon destructing the (local var) filehandle upon leaving the routine. I made that more explicit now, also removing the parse-time only properties.
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.
I like the use of generators, but I wonder if this cannot be streamlined a bit.
If I understand correctly,
_cleanup
is called here first, because we cannot know if thefunc
from a previous iteration didn't install some newkeylevels
. Can we not require that by providing a method on the base parser that allows temporary modification ofkeylevels
but also does the clean up as a context manager. The parser functions/generatorsfunc
can then use this manager around theiryield
statement.and sub classes would use this
If we also require all keylevel functions to be generators, the hot part of the loop can then just be
The back and forth between the parser generator functions and the base class is not quite clear to me, so maybe I'm missing some steps.
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.
Do these need to be attributes or can they be just local variables? They are only used to instantiate
Atoms
one L52 and below, or not?