Remove assigned but never used local variables. Enable Travis checking for unused local variables. #12284

practicalswift commented at 11:06 AM on January 28, 2018: contributor

Remove assigned but never used local variables. Enable Travis checking for unused local variables.

fanquake added the label Tests on Jan 28, 2018

fanquake requested review from jnewbery on Jan 29, 2018

promag commented at 1:21 PM on January 29, 2018: member

utACK 3f281a8.

jnewbery commented at 2:16 PM on January 29, 2018: member

Seems reasonable. On master:

→ flake8 --select F841 test/functional/feature_block.py 
test/functional/feature_block.py:661:9: F841 local variable 's' is assigned to but never used
test/functional/feature_block.py:698:9: F841 local variable 'b51' is assigned to but never used
test/functional/feature_block.py:816:9: F841 local variable 'b58' is assigned to but never used
test/functional/feature_block.py:823:9: F841 local variable 'b59' is assigned to but never used
test/functional/feature_block.py:864:9: F841 local variable 'b62' is assigned to but never used
test/functional/feature_block.py:1091:9: F841 local variable 'b74' is assigned to but never used
test/functional/feature_block.py:1104:9: F841 local variable 'b75' is assigned to but never used
test/functional/feature_block.py:1115:9: F841 local variable 'b76' is assigned to but never used

However, I've completely refactored this test in #11773 (which builds on #11771). My preference would be for those to be reviewed and merged.

I think removing all 'unused variable' warnings and enforcing F841 in our travis linter is a good longer term goal, but I'm concept NACKish on PRs that just fix style in individual files.

practicalswift force-pushed on Jan 29, 2018

practicalswift renamed this:
~~tests: Remove assigned but never used local variables~~
Remove assigned but never used local variables
on Jan 29, 2018

practicalswift renamed this:
~~Remove assigned but never used local variables~~
Remove assigned but never used local variables. Enable Travis checking for unused local variables.
on Jan 29, 2018

practicalswift commented at 2:53 PM on January 29, 2018: contributor

@jnewbery Good point. Now fixing it globally and enabling Travis CI checking via contrib/devtools/lint-python.sh. If your PR:s are merged before this PR I'll rebase on top of them.

Please re-review :-)

jnewbery commented at 3:57 PM on January 29, 2018: member

I'm still NACKish. Rebasing #11773 on this will be a lot of wasted work.

F841 is a style issue that may hide bugs, but none of the changes here are actual bug fixes. I think it's a good long-term goal, but not worth opening individual PRs.

practicalswift commented at 4:21 PM on January 29, 2018: contributor

@jnewbery If I rebase this PR on top of master once #11773 is merged, then this should be an ACK, no?

Assuming enforcing F841 in our Travis linter is a good long term goal.

jnewbery commented at 4:41 PM on January 29, 2018: member

I agree that adding F841 should be a long-term goal.

If I rebase this PR on top of master once #11773 is merged, then this should be an ACK, no?

Yes, but with the same caveats as #12295 - that I don't know the least disruptive way to fix lint warnings and add rules to Travis.

practicalswift force-pushed on Feb 8, 2018

practicalswift commented at 4:04 PM on February 8, 2018: contributor

Rebased on top of master now that #12295 has been merged! :-)

practicalswift force-pushed on Mar 11, 2018

fanquake requested review from MarcoFalke on Mar 14, 2018

MarcoFalke commented at 1:29 PM on March 18, 2018: member

I will look at this after #11773 is merged.

Remove assigned but never used local variables 169f3e8637

practicalswift force-pushed on Mar 29, 2018

practicalswift commented at 3:48 PM on March 29, 2018: contributor

#11773 is now merged. Please re-review :-)

MarcoFalke commented at 3:51 PM on March 29, 2018: member

utACK 22d1bd1914651f3c00739652471aabf5374ef092

in contrib/devtools/lint-python.sh:25 in 22d1bd1914 outdated

  21 | @@ -22,6 +22,7 @@
  22 |  # E275 missing whitespace after keyword
  23 |  # E304 blank lines found after function decorator
  24 |  # E306 expected 1 blank line before a nested definition
  25 | +# F401: module imported but unused

jnewbery commented at 3:54 PM on March 29, 2018:

This has snuck in somehow. Please remove! (F401 is documented lower down in the right place)

practicalswift commented at 4:02 PM on March 29, 2018:

Oh, thanks. Now fixed.

jnewbery commented at 3:54 PM on March 29, 2018: member

utACK 22d1bd1914651f3c00739652471aabf5374ef092 after removing the stray comment in lint-python.sh

practicalswift force-pushed on Mar 29, 2018

practicalswift commented at 4:02 PM on March 29, 2018: contributor

Updated. Please re-review :-)

practicalswift force-pushed on Mar 29, 2018

Enable flake8 warning F841 ("local variable 'foo' is assigned to but never used") ea04bf7862

practicalswift force-pushed on Mar 29, 2018

jnewbery commented at 5:24 PM on March 29, 2018: member

utACK ea04bf786263eb0c933648bce43627c0de4d84ef

MarcoFalke commented at 5:25 PM on March 29, 2018: member

utACK ea04bf786263eb0c933648bce43627c0de4d84ef

MarcoFalke merged this on Apr 1, 2018

MarcoFalke closed this on Apr 1, 2018

MarcoFalke referenced this in commit fa5825d610 on Apr 1, 2018

jasonbcox referenced this in commit 4afd5d82e9 on Aug 9, 2019

PastaPastaPasta referenced this in commit f1b26d5fbe on Sep 27, 2020

PastaPastaPasta referenced this in commit 1b4300ffdb on Oct 22, 2020

PastaPastaPasta referenced this in commit 7de10e4737 on Oct 27, 2020

practicalswift deleted the branch on Apr 10, 2021

gades referenced this in commit ba2ba94022 on Apr 20, 2022

DrahtBot locked this on Aug 16, 2022