Skip to content

Optimize handling of long lines for checkers like 'missing-final-newline' #5925

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

Merged
merged 5 commits into from
Mar 16, 2022
Merged

Conversation

skirpichev
Copy link
Contributor

  • Add yourself to CONTRIBUTORS.txt if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature, or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Type of Changes

Type
🐛 Bug fix

Description

A replacement for #5786 per suggestion of @DanielNoord. Fix catastrophic backtracking in OPTION_RGX.

Closes #5724

@coveralls
Copy link

coveralls commented Mar 16, 2022

Pull Request Test Coverage Report for Build 1992225427

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.078%

Totals Coverage Status
Change from base Build 1991292672: 0.0%
Covered Lines: 15188
Relevant Lines: 16144

💛 - Coveralls

@skirpichev
Copy link
Contributor Author

CI seems to be broken for the main branch.

@DanielNoord DanielNoord self-requested a review March 16, 2022 07:20
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @skirpichev, I got caught up in other things and haven't had the time to look at this.

Some minor changes to the comments and structure of the pattern. We might be able to sneak this in 2.13 👍

Edit: Btw, CI should pass again as I merged the PR that fixed it.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Mar 16, 2022
@skirpichev
Copy link
Contributor Author

Ok, tests pass and, I hope, I don't forget anything from the requested changes.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @skirpichev for you work and the quick responses to my review! LGTM!

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you ! And congratulation on becoming a pylint contributor :)

@Pierre-Sassoulas Pierre-Sassoulas merged commit ccd32d3 into pylint-dev:main Mar 16, 2022
@skirpichev skirpichev deleted the fix-5724 branch March 16, 2022 12:14
@skirpichev
Copy link
Contributor Author

BTW, there are two context managers to handle timeouts (in tests/checkers/unittest_refactoring.py and in tests/test_regr.py). Probably, one from definitions could be reused (if you don't like pytest-timeout).

@Pierre-Sassoulas
Copy link
Member

Nice catch ! I have nothing against using external pytest plugins. They probably did not exist when the context manager were first created.

@DanielNoord
Copy link
Collaborator

Yeah, I need to move those to conftest. It's on my (long) todo list 😄 Feel free to open a PR to do so!

@skirpichev
Copy link
Contributor Author

skirpichev commented Mar 17, 2022 via email

@DanielNoord
Copy link
Collaborator

On Thu, Mar 17, 2022 at 01:02:40AM -0700, Daniël van Noord wrote: Yeah, I need to move those to conftest. It's on my (long) todo list 😄 Feel free to open a PR to do so!
I would suggest using pytest-timeout instead. Any objections?

None, other than that I haven't worked with it before so when I first made a "timeout test" myself for pylint I just built on what was already there. Didn't really have the time (or desire) at the time to learn a new tool. But I'd be happy to review a PR that adds it and learn from it myself 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'missing-final-newline' is surprisingly slow on a large codebase
4 participants