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
Enhancement: Add a [unnecessary-comprehension]-checker #2905
Comments
Sound good and useful, thanks for tackling this. |
I came up with the following testcases: # For name-reference see https://docs.python.org/3/reference/expressions.html#displays-for-lists-sets-and-dictionaries
# List comprehensions
[x for x in iterable] # [unnecessary-comprehension]
[y for x in iterable] # expression != target_list
[x for x,y,z in iterable] # expression != target_list
[(x,y,z) for x,y,z in iterable] # [unnecessary-comprehension]
[(x,y,z) for (x,y,z) in iterable] # [unnecessary-comprehension]
[x for x, *y in iterable] # expression != target_list
[x for x in iterable if condition] # exclude comp_if
[y for x in iterable for y in x] # exclude nested comprehensions
# Set comprehensions
{x for x in iterable} # [unnecessary-comprehension]
{y for x in iterable} # expression != target_list
{x for x,y,z in iterable} # expression != target_list
{(x,y,z) for x,y,z in iterable} # [unnecessary-comprehension]
{(x,y,z) for (x, y, z) in iterable} # [unnecessary-comprehension]
{x for x, *y in iterable} # expression != target_list
{x for x in iterable if condition} # exclude comp_if
{y for x in iterable for y in x} # exclude nested comprehensions
# Dictionary comprehensions
{k: v for k, v in iterable} # [unnecessary-comprehension]
{v: k for k, v in iterable} # key value wrong order
{k: v for (k, v) in iterable} # [unnecessary-comprehension]
{x: y for x,y,z in iterable} # expression != target_list
{x[0]: x[1] for *x in iterable} # [unnecessary-comprehension]
{x: y for x, y in iterable if condition} # exclude comp_if
{y: z for x in iterable for y, z in x} # exclude nested comprehensions Any feedback on the testcases? |
@pheanex just checking old notifications and saw this. I like this idea. 💖 I don't think this one should be seen as an unnecessary comprehension: {x[0]: x[1] for *x in iterable} I doubt that'd come up often, but it's possible that's an iterable of 3-item tuples, so |
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org>
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org>
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org>
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org>
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org> Reviewed-By: Christian Heimes <cheimes@redhat.com>
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org> Reviewed-By: Christian Heimes <cheimes@redhat.com>
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org>
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org> Reviewed-By: Christian Heimes <cheimes@redhat.com>
1) Lines 837, 844, 851, 873, 896, 915, 948 in model.py has been affected by new rule "Unnecessary use of a comprehension". Rationale for the changes can be found here: pylint-dev/pylint#2905 2) Line 84 in service.py has been affected by new rule "Unnecessary "else" after "break"". In our case exiting if block is granted by brake statement, therefore else is not necessary
1) Lines 837, 844, 851, 873, 896, 915, 948 in model.py has been affected by new rule "Unnecessary use of a comprehension". Rationale for the changes can be found here: pylint-dev/pylint#2905 2) Line 84 in service.py has been affected by new rule "Unnecessary "else" after "break"". In our case exiting if block is granted by brake statement, therefore else is not necessary
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org> Reviewed-By: Christian Heimes <cheimes@redhat.com>
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org>
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org>
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org> Reviewed-By: Christian Heimes <cheimes@redhat.com> Reviewed-By: Christian Heimes <cheimes@redhat.com>
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org>
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org>
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org>
Can anyone explain to me why |
Okay - just |
hmm.. |
@feluelle I'm not sure I understand your last comment, do you mean PyCharm complains on something like |
Newest Pylint introduced additional checks [1]: - import-outside-toplevel [2] > This check warns when modules are imported from places other than a module toplevel, e.g. inside a function or a class. - no-else-continue [3] > These checks highlight unnecessary else and elif blocks after break and continue statements. - unnecessary-comprehension [4] > This check is emitted when pylint finds list-, set- or dict-comprehensions, that are unnecessary and can be rewritten with the list-, set- or dict-constructors. [1] https://github.com/PyCQA/pylint/blob/pylint-2.4.0/doc/whatsnew/2.4.rst [2] pylint-dev/pylint#3067 [3] pylint-dev/pylint#2327 [4] pylint-dev/pylint#2905 Fixes: https://pagure.io/freeipa/issue/8077 Signed-off-by: Stanislav Levin <slev@altlinux.org>
@PCManticore @pheanex I am seeing the unnecessary-comprehension warning for the following code: labels = ['one', 'two', 'three']
indexed_labels = [(i, text) for i, text in enumerate(labels)] # warning here I am currently using pylint 2.4.4. I think it is a false positive. Otherwise, what are the better alternatives? |
@jdhao try
|
@feluelle Thanks for the tip. I find the longer version more readable and clear, though. |
@PCManticore @pheanex
|
@devenpatel2 don't you think using dict instead is simpler/easier to read?
it would be
|
Goodness me! I never tried that. This is so much elegant. Thank you. |
Seen in https://github.com/google/pytype/runs/2395958856?check_suite_focus=true. Reasoning behind this check seems to be pylint-dev/pylint#2905. I'm not entirely convinced that the comprehension is less readable, but this is easy enough to change. PiperOrigin-RevId: 369695810
I suggest adding a new [unnecessary-comprehension]-checker (name up for discussion).
Inspired by @treyhunner's blogpost Abusing and overusing list comprehensions in Python (Section "Using comprehensions when a more specific tool exists")
we could implement a checker that detects list/dict/set-comprehensions that can be replaced by the corresponding list/dict/set-constructors, which is faster and more readable.
For example:
list(iterable)
instead of[x for x in iterable]
dict(some_dict)
instead of{k: v for k, v in some_dict}
set(iterable)
instead of{e for e in iterable}
Although these cases seem trivial/obvious, I think having such a checker would help in cases where longer/more complex variable-names are used.
I'd be willing to implement the checker myself.
What do you think?
As a next step and if there are no basic objections I would add testcases here.
The text was updated successfully, but these errors were encountered: