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

C2901 & C2902: New checks for unnecessary lambda expression usage #6004

Merged
merged 24 commits into from
May 4, 2022

Conversation

jpy-git
Copy link
Contributor

@jpy-git jpy-git commented Mar 27, 2022

Type of Changes

Type
✨ New feature

Description

Closes #5976.

Adds new check unnecessary-lambda-assignment: Lambda expression assigned to a variable. Define a function using the "def" keyword instead.

From PEP8:

Always use a def statement instead of an assignment statement that binds a lambda expression directly to an identifier:
# Correct:
def f(x): return 2*x
# Wrong:
f = lambda x: 2*x

This is equivalent to E731 in flake8 but additionally addresses several false negatives that are missed by that implementation: PyCQA/pycodestyle#1061

Also adds a new check unnecessary-direct-lambda-call: Lambda expression called directly. Execute the expression inline instead.
i.e. a**2 not (lambda x: x**2)(a)
This prevents a weird redundant usage of lambda functions that I've seen some junior devs do before.

@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 27, 2022

There were a lot of existing tests using that I had to add ignores to in that last commit, so look at the commit before that for easier reviewing

@coveralls
Copy link

coveralls commented Mar 27, 2022

Pull Request Test Coverage Report for Build 2269685819

  • 27 of 27 (100.0%) changed or added relevant lines in 1 file are covered.
  • 10 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.007%) to 95.185%

Files with Coverage Reduction New Missed Lines %
pylint/checkers/stdlib.py 10 94.4%
Totals Coverage Status
Change from base Build 2266524332: 0.007%
Covered Lines: 15855
Relevant Lines: 16657

💛 - Coveralls

@DanielNoord DanielNoord self-requested a review March 28, 2022 08:20
@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Mar 28, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.0 milestone Mar 28, 2022
Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

This is hilariously unlikely to happen, but what do you think about attempting to catch this case? (It doesn't emit a warning even if you remove the disable):
https://github.com/PyCQA/pylint/blob/edf753e6864224ac0144c0ac12c8d89baa93e097/tests/functional/a/assignment/assignment_expression.py#L44

pylint/checkers/lambda_expressions.py Outdated Show resolved Hide resolved
pylint/checkers/lambda_expressions.py Outdated Show resolved Hide resolved
@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 29, 2022

@jacobtylerwalls I've extended the check to flag assignment via named expressions as well 👍
Currently the tests for python<=3.7 are failing because walrus operators were only introduced in 3.8. Is there a way to only run some lines/files for certain versions?

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Mar 29, 2022

Currently the tests for python<=3.7 are failing because walrus operators were only introduced in 3.8. Is there a way to only run some lines/files for certain versions?

Yes, look at a .rc files in tests/functional/ there should be examples of use for min_version

@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 29, 2022

Thanks!

@jpy-git jpy-git closed this Mar 29, 2022
@jpy-git jpy-git reopened this Mar 29, 2022
@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 29, 2022

(sorry about the closing and reopening btw, some of the checks keep timing out for no reason (seems like a GHA thing rather than the code changes and closing/reopening is the easiest way to kick it off again)

@Pierre-Sassoulas
Copy link
Member

seems like a GHA thing rather than the code changes and closing/reopening is the easiest way to kick it off again

Yes, the python 3.8 + coverage job regularly takes more than 15mn. It's more frequent than an actual failed tests right now.

@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas Not sure if this is an option, but as a collaborator I can re-run actions. Can we allow this for contributors as well? That might make this more workable.

@Pierre-Sassoulas
Copy link
Member

Can we allow this for contributors as well?

I don't think so, but the trick to open/close the issue should do most of the time.

@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 29, 2022

is there a reason why we don't increase the timeout-minutes settings in the yaml file?

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Mar 29, 2022

From what I understand the pipeline that are stuck are stuck because the runner used by github has only one core and we try to use two (and we can't ask for multiple core so it's 'random'). So it will fail even if we wait one hour. 15mn permit to free the worker sooner and have overall better availability.

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, but I think it would be a nice check to inaugurate the false positive github action primer when it's available. It's a default checker and it's going to be run in a lot of code once it lands in pylint.

@jpy-git jpy-git closed this Apr 3, 2022
@jpy-git jpy-git reopened this Apr 3, 2022
@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas @jpy-git For some reason the latest merge of main broke this. Could either of you look into this?

After that, I think we can go ahead and merge this as we shouldn't let this be blocked by the eventual update to the primer imo.

@Pierre-Sassoulas
Copy link
Member

Sure let me fix this, I should have merged that a long time ago. I think the issue is only with the checker number that has been claimed by another new checker since.

jpy-git and others added 20 commits May 4, 2022 10:12
@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented May 4, 2022

@DanielNoord pypy has column/line variation compared to cpython. This is something we do not control and we're probably never going to change the parsing to have the right column in both. Should we still take the column/line into account under PYPY ?

@DanielNoord
Copy link
Collaborator

DanielNoord commented May 4, 2022

@DanielNoord pypy has column/line variation compared to cpython. This is something we do not control and we're probably never going to change the parsing to have the right column in both. Should we still take the column/line into account under PYPY ?

Let's ignore this test on PyPy. This is a very rare (and only) case where the col_offset (and not end_col_offset) differ. Actually I think the column offset of PyPy is more "correct", but that's just my opinion 😄 . I don't think we should stop checking col_offset on PyPy all together as this is the only case where it differs.

@DanielNoord DanielNoord merged commit 192da67 into pylint-dev:main May 4, 2022
@DanielNoord
Copy link
Collaborator

Thanks @jpy-git for contributing the new checker and thanks to all reviewers 🎉

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.

New lint: Use def instead of lambda expression for bare assignments and calls
5 participants