- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 425
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
Use isinstance() checks in expcetions match_examples() #1065
Use isinstance() checks in expcetions match_examples() #1065
Conversation
lark/exceptions.py
Outdated
# Try exact match first | ||
if ut.token == self.token: # type: ignore[attr-defined] | ||
logger.debug("Exact Match at example [%s][%s]" % (i, j)) | ||
return label | ||
|
||
if token_type_match_fallback: | ||
# Fallback to token types match | ||
if (ut.token.type == self.token.type) and not candidate[-1]: | ||
if (ut.token.type == self.token.type) and not candidate[-1]: # type: ignore[attr-defined] | ||
logger.debug("Token Type Fallback at example [%s][%s]" % (i, j)) | ||
candidate = label, True |
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.
Since this and the previous block are both attempting to achieve the same goal, it is odd that they use different strategies (checking first vs letting it fail). It would probably be "better" pick a single strategy and use it for both, but I wanted to keep this PR to just addressing the mypy errors instead of changing code.
I don't like the idea of adding workarounds to solve mypy's temporary deficiencies. But I agree we should be consistent in our approaches in the code.
You mean isinstance on UnexpectedInput? I'm okay with that. I don't think it would be a breaking change. @MegaIng , opinion? |
Yes. The first block that deals with I agree that it likely should not be considered a breaking change. Reading the docstring for |
@plannigan Just a reminder that changes are needed for this PR to be accepted. |
5f4dac6
to
5eae1ec
Compare
I guess I had gotten distracted by the other PRs that I forgot this was still open and needed the small update. Should be good now. |
No worries. I appreciate that you're taking the time to contribute! |
This PR adds ignore comments as the
hasattr(x, 'foo')
strategy is something mypy could reason about statically.An alternate option would be to use
isinstance()
checks for the exceptions in the file that define that attributes. I opted to no do that as it would be a subtle change in behavior. The current implementation supports handling any arbitrary subclass ofUnexpectedInput
with these attributes (even if there was not an explicit intent to support that functionality).