Skip to content
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

Merged
merged 1 commit into from
Feb 27, 2022

Conversation

plannigan
Copy link
Contributor

@plannigan plannigan commented Jan 4, 2022

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 of UnexpectedInput with these attributes (even if there was not an explicit intent to support that functionality).

Comment on lines 117 to 131
# 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
Copy link
Contributor Author

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.

@erezsh
Copy link
Member

erezsh commented Jan 4, 2022

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.

An alternate option would be to use isinstance() checks for the exceptions

You mean isinstance on UnexpectedInput? I'm okay with that. I don't think it would be a breaking change.

@MegaIng , opinion?

@plannigan
Copy link
Contributor Author

You mean isinstance on UnexpectedInput? I'm okay with that. I don't think it would be a breaking change.

Yes. The first block that deals with accepts would be isinstance(x, UnexpectedToken). The second block that deals with token would be isinstance(x, (UnexpectedToken, UnexpectedEOF)).

I agree that it likely should not be considered a breaking change. Reading the docstring for match_examples(), while not explicit, it seems fairly clear that the expectation is that it only supports exceptions that are part of lark. The fact that it did support other custom sub-classes of UnexpectedInput is an implementation detail which can be changed.

@erezsh
Copy link
Member

erezsh commented Feb 21, 2022

@plannigan Just a reminder that changes are needed for this PR to be accepted.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@plannigan plannigan force-pushed the mypy_fix-exceptions_hasattr branch from 5f4dac6 to 5eae1ec Compare February 26, 2022 18:32
@plannigan plannigan changed the title Ignore mypy errors related to hasattr with literal attribute names Use isinstance() checks in expcetions match_examples() Feb 26, 2022
@plannigan
Copy link
Contributor Author

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.

@erezsh
Copy link
Member

erezsh commented Feb 26, 2022

No worries. I appreciate that you're taking the time to contribute!

@erezsh erezsh merged commit bc22192 into lark-parser:master Feb 27, 2022
@plannigan plannigan deleted the mypy_fix-exceptions_hasattr branch February 28, 2022 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants