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

spellcheck will skip the rule names of mypy inline directives #5929

Merged

Conversation

ejfine
Copy link
Contributor

@ejfine ejfine commented Mar 16, 2022

Type of Changes

Type
🐛 Bug fix

Description

The spellchecker was a bit overaggressive in flagging the Mypy rule names inside of ignore directives. This allows those to be ignored by the spellchecker.

@ejfine
Copy link
Contributor Author

ejfine commented Mar 16, 2022

@Pierre-Sassoulas - you were the one who took a look at my previous PR regarding the spellchecker (#4330), are you still the right person to review this?

@ejfine ejfine force-pushed the spellcheck-ignore-mypy-rule-names branch from 4a937f5 to c7deb4d Compare March 16, 2022 16:19
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.0 milestone Mar 16, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Needs review 🔍 Needs to be reviewed by one or multiple more persons Enhancement ✨ Improvement to a component labels Mar 16, 2022
@coveralls
Copy link

coveralls commented Mar 16, 2022

Pull Request Test Coverage Report for Build 2278156268

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 95.289%

Totals Coverage Status
Change from base Build 2277828137: 0.002%
Covered Lines: 15959
Relevant Lines: 16748

💛 - Coveralls

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.

Sorry for the time it took to review. It looks pretty good already. I wanted to ask you to add a functional tests but I'm struggling to add one myself in #6137. I'll upgrade your own MR when I figure out how to do that.

@Pierre-Sassoulas Pierre-Sassoulas added Blocked 🚧 Blocked by a particular issue and removed Needs review 🔍 Needs to be reviewed by one or multiple more persons labels Apr 2, 2022
@DanielNoord
Copy link
Collaborator

@eli88fine Sorry for letting this rest so long. I think @Pierre-Sassoulas and I agreed that this shouldn't be blocked by our internal changes to how we test the spelling checker.

That said, would you be willing to explore whether this regex matching can be done in the same pattern as above? regex matching and compiling is often quite expensive (it is on of our main costs during start-up) so I'd like to integrate most patterns where possible.
If not, please let me know and I might have some time to tinker with this myself!

@ejfine
Copy link
Contributor Author

ejfine commented May 5, 2022

@DanielNoord - what's the "in the same pattern as above" that you're referring to? I don't see a link

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.

Let's get this merged 😄

pylint/checkers/spelling.py Show resolved Hide resolved
pylint/checkers/spelling.py Outdated Show resolved Hide resolved
pylint/checkers/spelling.py Outdated Show resolved Hide resolved
ChangeLog Show resolved Hide resolved
@ejfine
Copy link
Contributor Author

ejfine commented May 5, 2022

trying to follow the timestamps on all the different threads. Are you saying "leave the code alone, just add an entry in the doc/whatsnew" ?

Or are there actual code changes you think would be good?

@DanielNoord
Copy link
Collaborator

trying to follow the timestamps on all the different threads. Are you saying "leave the code alone, just add an entry in the doc/whatsnew" ?

Or are there actual code changes you think would be good?

Sorry! Being indecisive today 😅

The last comments were all recent. So let's change the pattern slightly and add the changelog entry!

@ejfine ejfine force-pushed the spellcheck-ignore-mypy-rule-names branch from c7deb4d to 174a05c Compare May 5, 2022 19:31
@ejfine
Copy link
Contributor Author

ejfine commented May 5, 2022

regex has been changed and docs updated

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.

One final change and then LGTM!

ChangeLog Show resolved Hide resolved
@DanielNoord DanielNoord removed the Blocked 🚧 Blocked by a particular issue label May 5, 2022
Co-authored-by: Daniël van Noord <[email protected]>
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.

Thank you for your contribution @eli88fine ! It's going to be released in 2.14.

@DanielNoord DanielNoord merged commit ea4e69b into pylint-dev:main May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants