-
-
Notifications
You must be signed in to change notification settings - Fork 435
Remove redefinitions related to standalone parser #1115
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
Remove redefinitions related to standalone parser #1115
Conversation
plannigan
commented
Feb 16, 2022
- Use a dictionary to look up valid parser creators
- Split parsing frontend argument validatoin from parsing frontend creation
- Convert MakeParsingFrontend uses into functions
_ParserArgType: 'TypeAlias' = 'Literal["earley", "lalr", "cyk", "auto"]' | ||
_LexerArgType: 'TypeAlias' = 'Union[Literal["auto", "basic", "contextual", "dynamic", "dynamic_complete"], Type[Lexer]]' |
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.
Extract type definition to a place where they can be used in multiple places. Since we aren't making typing_extensions
a runtime dependencies, the definitions need to be a string to defer the evaluation. Which then requires the definitions to be explicitly marked as TypeAlias
s, requiring an additional typing_extensions
usage during type checking.
@@ -24,6 +37,7 @@ class LexerConf(Serialize): | |||
g_regex_flags: int | |||
skip_validation: bool | |||
use_bytes: bool | |||
lexer_type: Optional[_LexerArgType] |
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.
Wasn't initially annotated and the initial assignment to None
made mypy
think the value could only be None
.
def __init__(self, parser_type, lexer_type): | ||
self.parser_type = parser_type | ||
self.lexer_type = lexer_type | ||
def _deserialize_parsing_frontend(data, memo, lexer_conf, callbacks, options): |
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.
MakeParsingFrontend.deserialize()
wasn't using the instance variables, so it could be converted into a standalone function.
'earley': create_earley_parser, | ||
'cyk': CYK_FrontEnd, | ||
}[parser_conf.parser_type] | ||
create_parser = _parser_creators.get(parser_conf.parser_type) |
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.
This previously would always find a value for the given key, but then would raise an exception when NotImplemented
was used as a Callable
(which isn't supported). Now there is a descriptive exception when the given key isn't supported.
_parser_creators['cyk'] = CYK_FrontEnd | ||
|
||
|
||
def _construct_parsing_frontend( |
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.
MakeParsingFrontend.__call__()
was using the instance variables. However the only use of a class was in a single function, which immediately used it as a Callable
. So I feel like it is cleaner to make all of the values arguments to a single function.
It looks alright to me. One question - how do you get to the exception "not supported in standalone mode" ? In realistic use |
The exception could I think instead also be an |
Yeah, in standalone mode you would need to do what you described or explicitly call |
* Use a dictionary to look up valid parser creators * Split parsing frontend argument validatoin from parsing frontend creation * Convert MakeParsingFrontend uses into functions
31a2b38
to
074c55b
Compare