Fix disabling of fixme
#7144
Fix disabling of fixme
#7144
Conversation
DanielNoord
added
Bug 🪲
Needs backport
Pull Request Test Coverage Report for Build 2631234011
💛 - Coveralls |
This comment has been minimized.
This comment has been minimized.
| comment_text = comment.string[1:].lstrip() # trim '#' and white-spaces | ||
|
|
||
| # handle pylint disable clauses | ||
| disable_option_match = OPTION_PO.search(comment_text) |
There was a problem hiding this comment.
I don't understand what this code is doing well. We were ignoring fixme if there was something looking like a pylint disable comment ? 😕
i.e. pylint: disable=fixme TODO or pylint: disable=no-member TODO should not raise ?
There was a problem hiding this comment.
Yeah, we were seeing if there is a # pylint: disable=fixme on the same line as the fixme comment. However, we have other code that does this for us and it doesn't allow using disable-next.
Just removing is fine.
There was a problem hiding this comment.
I don't see the part where it's specific to "fixme" though, wasn't it a special case where you could add TODO in pylint disable ? (Someone too lazy to do a grep for "pylint: disable" at some point, but not lazy enough to not add TODO everywhere ?)
There was a problem hiding this comment.
I don't think it actually does anything other than disallow disable. PragmaParserError probably catches all other issues.
The fact that all of our enable tests work without this shows that it is completely unnecessary 😄
There was a problem hiding this comment.
Sorry for asking but this cleanup looked too good to be true 😄 I'm just amazed at the number of time we just remove a bunch of code to fix a bug. It's been happening over and over.
There was a problem hiding this comment.
😄
I also was quite surprised when I found this code. At first I though for some reason the fixme checker was responsible for parsing all pragma's but then I remembered that I actually refactored that code myself and made sure that it will always be PyLinter ... 😅
There was a problem hiding this comment.
Having refactored half the codebase has its perks 😄
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
| @@ -1 +0,0 @@ | |||
| You can help us make the doc better `by contributing <https://github.com/PyCQA/pylint/issues/5953>`_ ! | |||
There was a problem hiding this comment.
It's to get crafty with regex to force the presence of a ticket number, for example see #2874 (comment)
There was a problem hiding this comment.
I'm suggesting to add that text in detail.rst, but it's impossible to suggest on remove file :)
(My pylint dev machine just died, I lost a lot of unpushed branch :( )
There was a problem hiding this comment.
Oh not. That sounds bad... Might be able to recover some from your fork?
There was a problem hiding this comment.
Yeah some branches I pushed semi regularly, everything experimental or unfinished though... (especially a massive invalid-name refactor for name that were too small...) Well at least I won't have to clean up the git repo anymore and I get to start from a clean slate 🥲 I had a disk space problem for some time, and after months of just deleting the cache from time to time I think I finally had something important that did not get written correctly (auth app crashed) 😅
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
This comment has been minimized.
This comment has been minimized.
|
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit 31bcdd6 |
Pierre-Sassoulas
added
Backported
and removed
Needs backport
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
doc/whatsnew/<current release.rst>.Type of Changes
Description