Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add typing to
NodeNG.statement
#1217Add typing to
NodeNG.statement
#1217Changes from 20 commits
d8347f0
f9fe68d
7c47926
b6373e3
19c5ed4
baef395
005a9ff
da1b989
d95d5ac
4a9af1e
ef019e6
1f6ec98
df9b49c
6026560
d07374c
2596ee6
e8d0ff2
73fb21d
6777dc9
1900532
11aefd2
2b6e7d0
cab6a88
78588ff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It seems like our luck didn't outran us yet. This actual now triggers a
useless-suppression
message. Guess we found afalse-negative
by adding*
😅 pylint-dev/pylint#5270There 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.
Is
useless-suppression
not enabled forastroid
? CI is passing.We might want to enable that in another PR.
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.
It's enabled, but not a failure condition! I don't think we should change that.
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.
Sure? Seems like something that will be easily missed in review
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.
I also think we should make it a failure condition so it's handled in the proper MR each time.
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.
It would be as easy as adding
--fail-on=useless-suppression
to thepylint
command.The issue is that some errors might be Python version specific. It could create a situation where only one,
pre-commit
orCI
, will succeed when tested with different versions. Thepy-version
option should help, but I don't know if it's enough.Additionally, this would prevent us from having the latest
pylint
version installed in a local env as this is likely to have fixed some false-positives. -> Which would then triggeruseless-suppression
.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.
That last point will indeed be difficult to avoid. I'll try and think of something to fix that, but in the meantime let's let this rest!
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.
Do you maybe want to track this as issue in
pylint
?I completely agree!
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.
Shouldn't this be tracked in
astroid
?pylint
has its own issues withuseless-suppression
(pylint-dev/pylint#5040) 😄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.
😂 Though if we enable it, we'll likely want to do it for
pylint
too. Tbh I don't really care about it due to the issues mentioned earlier.