-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Fix performance in get_imports regexp #34298
Fix performance in get_imports regexp #34298
Conversation
It is not a typo but still a small change with existing tests |
This looks good now! It should be simpler and more performant, and shouldn't change which strings it matches. I have one more suggestion, though. This regex looks like it would fail in cases when the strings "try" and "except" occur internally in some lines. This is probably very rare, but maybe we should change the regex to To be clear, this isn't a bug with your PR - it's an issue with the regex that already existed! |
Any regex should fail because even valid Python source is not defined by a regular language. """
try:
pass
except:
pass
"""
# code in multi line string literal Should I add it in another PR with more tests? This PR is more about performance improvements. Maybe we can wait for @gante to reply, because he wrote this lines. |
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.
@AlekseyLobanov yes, good point - we can keep this PR simple. Want me to merge this one now, and if you think you can improve it further, you can open a new PR with added test(s)?
Can we just merge this for now? It will be great. |
Yes, the diff looks good to me. Thanks for the PR! |
What does this PR do?
Just found that one of the Regular Expressions may be very slow (O(N^3)) on some special inputs.
Something like
'try:' + ' ' * 1500
may take a lot of time. I replaced it with the same logic but fasterBefore submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@gante