Fix false positive when testing for-loops for unbalanced unpacking (W0644) #8892
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
β¦mber of targets to the size of each value, instead of the number of values, since the for loop will iterate the values and unpack each in turn onto the targets.
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
β¦king. The original implementation would use `NodeNG.get_children()` which would return the AST subnodes for the Dict (i.e. all the keys and values), and not just the keys.
β¦packing. Star unpacking requires there to be at least as many values to unpack as targets, so the unpacking is considered valid if the number of targets equal the number of values or if it's a star-unpacking with at least as many values as targets
β¦thub.com/Neowizard/pylint into false-positive-unbalanced-dict-unpacking
for more information, see https://pre-commit.ci
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8892 +/- ##
=======================================
Coverage 95.75% 95.76%
=======================================
Files 173 173
Lines 18604 18639 +35
=======================================
+ Hits 17815 17850 +35
Misses 789 789
|
β¦ced-dict-unpacking to complete test code coverage
This comment has been minimized.
This comment has been minimized.
β¦thub.com/Neowizard/pylint into false-positive-unbalanced-dict-unpacking
This comment has been minimized.
This comment has been minimized.
β¦value`. Also refactored the code a bit for readability
Pierre-Sassoulas
added
False Positive π¦
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
β¦thub.com/Neowizard/pylint into false-positive-unbalanced-dict-unpacking
for more information, see https://pre-commit.ci
β¦thub.com/Neowizard/pylint into false-positive-unbalanced-dict-unpacking
Pierre-Sassoulas
added
the
Needs review π
mbyrnepr2
reviewed
Aug 15, 2023
mbyrnepr2
reviewed
Aug 15, 2023
mbyrnepr2
reviewed
Aug 15, 2023
Pierre-Sassoulas
requested changes
Aug 16, 2023
β¦lanced-dict-unpacking
β¦ables.py`. Also, noted in unit-test that unbalanced-dict-unpacking is supressed for cases of dict-items-missing-iter
for more information, see https://pre-commit.ci
|
@mbyrnepr2, let me know if my latest commit is what you had in mind |
|
Yes looks great @Neowizard π |
mbyrnepr2
reviewed
Aug 30, 2023
β¦unpacking Co-authored-by: Mark Byrne <[email protected]>
mbyrnepr2
previously approved these changes
Aug 30, 2023
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-maintenance/2.17.x maintenance/2.17.x
# Navigate to the new working tree
cd .worktrees/backport-maintenance/2.17.x
# Create a new branch
git switch --create backport-8892-to-maintenance/2.17.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7b72c392f85aa15d26acd7448fed830e0fa97656
# Push it to GitHub
git push --set-upstream origin backport-8892-to-maintenance/2.17.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-maintenance/2.17.xThen, create a pull request where the |
|
@Pierre-Sassoulas How far are we to releasing |
|
I would say still pretty far (we did not start #3512 or anything in https://github.com/pylint-dev/pylint/issues?q=is%3Aopen+is%3Aissue+label%3A%22Blocker+%F0%9F%99%85%22 yet). But if something is really hard to backport we're not obligated to do it (hard defined as "massive cherry-pick conflict" right now but it could evolve to "any cherry-pick conflicts" if we want) |
Pierre-Sassoulas
added
Needs backport
|
(Added the "needs backport" label in order to remember that at 2.17.6 release time, it does not mean that I think we NEED to backport :) ) |
|
Thanks! I'm fine with backporting as well, but can't check out |
Pierre-Sassoulas
added
Backported
and removed
Needs backport
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Type of Changes
Description
When checking for-loops for unbalanced dict unpacking, Pylint would compare the length of the targets with the length of the values iterable. However, since the for-loop would iterate the values and unpack each value in turn, Pylint should compare the length of the targets with the length of each value (and not the total length of values).
Closes #8156