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 consider-using-assignment-expr
to CodeStyleChecker
#4876
Conversation
Pull Request Test Coverage Report for Build 1181085932
💛 - Coveralls |
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 would replace all reference to 'assignment expression' by 'walrus operator', this is the description I've seen in all python ressources talking about this personally.
--
https://www.python.org/dev/peps/pep-0572/#abstract -- |
80b12c4
to
a44d486
Compare
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.
The checker itself is nice and could go in 2.11.
pylint/extensions/code_style.py
Outdated
1, | ||
) | ||
suggestion = f"if {test_str}:" | ||
if len(suggestion) > 88: |
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 imagine 88 is the length of a line for black. The problem with that is that not everyone is using black, we also have a settings ourselves for lines too long that we could use. If we really want to do this I imagine we should behave differently if line-too-long is disabled or not and we should review all the checkers to see if it would also make sense... This would become very complicated and time intensive very quick. But beside that I think it's counterintuitive to have a message change if the line is shorter or longer. I don't think we want to surprise user like that. I know I suggested this idea in the ternary checker review and I think it made sense at the time because long ternary are awful but this would create confusion and makes pylint behave unexpectedly. I think it's more reasonable to let the user know all the time that they can use a walrus operator, and if the line become too long because of it, they could refactor it so it's shorter.
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 haven't pushed that latest changes yet as I'm still working on it. There I added a new config value so users can change it.
If we really want to do this I imagine we should behave differently if line-too-long is disabled or not and we should review all the checkers to see if it would also make sense...
I don't think that is necessary. Personally, I consider assignment expressions a special case as they are a truly optional feature that should only be used where it makes sense. It is completely independent from line-too-long
.
Furthermore, I can't thing of any specific existing and none optional check that could benefit from such a setting. At least not of the top of my head. Once we add it and users would expect it to work for check x / y that doesn't work yet, I'm sure we would see requests for it and can change it then.
But beside that I think it's counterintuitive to have a message change if the line is shorter or longer. I don't think we want to surprise user like that. [...] I think it's more reasonable to let the user know all the time that they can use a walrus operator, and if the line become too long because of it, they could refactor it so it's shorter.
Again, I would disagree here. A refactor is not always possible (or easy do accomplish). It's much more helpful to only recommend it in cases where there is a clear benefit from using it. It's just a recommendation for an optional feature after all.
One example, I think users won't like it if we recommend the use of :=
, but it results in an additional line because black reformatted it. Now they will need to add a pylint: disable
. If they do it two, three times, they will just disable the check completely and don't get anything from it. That doesn't help anyone.
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 is completely independent from line-too-long.
I don't think a checker that change behavior based on line length should be separated from our max-line-length option. If the value is 120 in max-line-length it means the user want to make 120 char lines. It would be strange to not raise a warning because more than 88 char is considered too much by this checker, when the user already explicitly said that 120 is okay.
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.
Two sides I would like to emphasis here:
-
max-line-length
>max-line-length-suggestions
Although the assignment expression would fit, it might not as easily readable. You have to consider that long lines especially with lots of brackets add quite a bit of complexity. The idea behind the suggestion in contrast is to make code better readable which is far easier accomplished by a shorter line. -
max-line-length
<max-line-length-suggestions
I would like to refer back to your point from the beginning: Why shouldn't we just warn about all cases? It can be formatted after the fact to fit within the max-line-length.
IMO both of these cases are valid arguments to decouple max-line-length
and max-line-length-suggestions
. With a separate setting users are free to choose how they would like to customize it. My personal recommendation btw would be around 72
characters.
That is my intention 🙂 |
@Pierre-Sassoulas Took me a bit longer than expected, but I think it's ready for review now. Let me know what you think |
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 only have minor comment, great work !
|
||
|
||
# ----- | ||
# Don't emit warning if match statement would be a better fit |
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.
But we're not emitting warning for match statement yet 😄 Is that a glimpse of your future plan ? 😛
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 might be 😄
But tbh I like it a bit better without :=
in this case. There is a certain pattern in it that makes it beautiful (and better readable).
o1 = 2
if o1 == 1:
...
elif o1 == 2:
...
elif o1 == 3:
...
tests/functional/ext/code_style/code_style_consider_using_assignment_expr.txt
Show resolved
Hide resolved
@cdce8p @Pierre-Sassoulas This check is enabled for the Pylint codebase, right? Shouldn't it have flagged a bunch of stuff? I was hoping to see what would turn up, but it looks like it isn't running. |
It's because we're supporting Python 3.6 :) |
Type of Changes
Description
Add
consider-using-assignment-expr
check toCodeStyleChecker
extension.In this basic example brackets aren't really necessary. However I would still like to suggest them, as this will prevent errors in the future if the test changes.
Since the assignment expression requires Python 3.8, I modified the existing
py-version
setting from theTypingChecker
extension, to be a global setting in the[MASTER]
section. The current implementation is fully backwards compatible. It doesn't matter in which sectionpy-version
is defined. If not set, this will default to the current interpreter version.The config change is already quite isolated. It wouldn't be an issue to create a separate PR for that if we want that.
Closes #4862