tests: Add Python dead code linter (vulture) to Travis #14365

practicalswift · 2018-10-02T10:53:47Z

Add Python dead code linter (vulture) to Travis.

Rationale for allowing dead code only after explicit opt-in (via --ignore-names):

Less is more :-)
Unused code is by definition "untested"
Unused code can be an indication of bugs/logical errors. By making the contributor aware of newly introduced unused code it gives him/her an opportunity to investigate if the unused code they introduce is malignant or benign :-)
Unused code is hard to spot for humans and is thus often missed during manual review
YAGNI

Based on #14312 to make linter job pass.

practicalswift · 2018-10-02T11:23:48Z

I've tested this linter over the set of all open mergeable pull requests.

A total of 196 pull requests were tested:

One true positive was found (and reported!)
Zero false positives were found

A precision score of 100 percent :-)

promag

In my system it takes around 1 second so it's not a big deal.

I suggest closing #14312.

practicalswift · 2018-10-02T13:44:44Z

@promag Thanks for reviewing. Good idea! Done.

test/functional/test_framework/test_framework.py

practicalswift · 2018-10-03T08:12:24Z

@MarcoFalke Updated. Please re-review :-)

test/functional/test_framework/messages.py

@MarcoFalke

ff94da7 tests: Make appveyor run with --usecli (practicalswift) db01839 test: Add missing call to skip_if_no_cli() (practicalswift) Pull request description: Add missing call to `skip_if_no_cli()` as suggested by @MarcoFalke in bitcoin#14365. Tree-SHA512: b0a2ddfad0f81cc9544f63c4e490fb983d833a47c23522549d1200ea6a8a132b2cd4bf0d66b862ef3a548d8471128b80aea3525fb5dec65221e23f32a8d46746

promag · 2018-10-04T18:54:51Z

Should explicitly set --min-confidence? Currently it's 60.

practicalswift · 2018-10-04T20:56:59Z

@promag Judging from my testing of vulture over the set of the 196 mergeable open PR:s the default of 60 seems reasonable. Should we keep the default for now?

promag · 2018-10-04T21:07:05Z

My suggestion is to add --min-confidence 60.

Also, from the vulture documentation:

We recommend using whitelists instead of --ignore-names or --ignore-decorators whenever possible

WDYT?

practicalswift · 2018-10-04T21:52:55Z

@promag I've not added an explicit --min-confidence 60.

Regarding the white-list method: I'm afraid a whitelist replicating the current wildcard based method would be quite long. And perhaps burdensome to keep updated.

The trade-off looks like this:

    --ignore-names "argtypes,connection_lost,connection_made,converter,data_received,\
        daemon,errcheck,msg_generic,on_*,optionxform,restype,skip_if_no_cli"

Versus:

$ vulture --min-confidence 60 --make-whitelist $(git ls-files -- "*.py" ":(exclude)contrib/") | wc -l
110

Despite the recommendation I think --ignore-names might be the better option for us since we need the wildcard functionality.

test/lint/lint-python-dead-code.sh

promag · 2018-10-04T21:57:35Z

@promag I've not added an explicit --min-confidence 60.

You have.

practicalswift · 2018-10-05T07:14:35Z

@promag I've not added an explicit --min-confidence 60.

You have.

Oh, "not" should have been "now" :-D

ken2812221 · 2018-10-06T16:44:50Z

utACK c422cf2b50b08b489172de28f69083784c35a455

jb55 · 2018-10-08T11:19:03Z

utACK c422cf2b50b08b489172de28f69083784c35a455

conscott · 2018-10-12T15:38:43Z

utACk c422cf2b50b08b489172de28f69083784c35a455 - vulture is great!

practicalswift · 2018-10-16T12:19:34Z

Added a commit which pins the vulture version to avoid the possibility of having the build broken if a new vulture version being released.

Please re-review :-)

DrahtBot · 2018-10-27T19:31:08Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14505 (Make all single parameter constructors explicit (C++11) by practicalswift)
#13728 (Scripts and tools: Run the CI lint stage on mac by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

test/functional/test_framework/key.py

maflcko · 2018-11-07T17:22:26Z

Going to merge this since it had 3 utACKs and is probably not worth to wait for all re-ACKs.

c82190c tests: Add Python dead code linter (vulture) (practicalswift) 590a57f tests: Remove unused testing code (practicalswift) Pull request description: Add Python dead code linter (`vulture`) to Travis. Rationale for allowing dead code only after explicit opt-in (via `--ignore-names`): * Less is more :-) * Unused code is by definition "untested" * Unused code can be an indication of bugs/logical errors. By making the contributor aware of newly introduced unused code it gives him/her an opportunity to investigate if the unused code they introduce is malignant or benign :-) * Unused code is hard to spot for humans and is thus often missed during manual review * [YAGNI](https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it) Based on #14312 to make linter job pass. Tree-SHA512: 4c581df7c34986e226e4ade479e0d3c549daf38f4a4dc4564b25564d63e773a1830ba55d1289c771b1fa325483e8855b82b56e61859fe8e4b7dfa54034b093b6

Summary: Removal of dead python code Partial backport of [[bitcoin/bitcoin#14365 | PR14365]] Commit bitcoin/bitcoin@590a57f Depends on D7981 (backported out of order) Test Plan: `ninja && ninja check-functional` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7987

…Travis c82190c tests: Add Python dead code linter (vulture) (practicalswift) 590a57f tests: Remove unused testing code (practicalswift) Pull request description: Add Python dead code linter (`vulture`) to Travis. Rationale for allowing dead code only after explicit opt-in (via `--ignore-names`): * Less is more :-) * Unused code is by definition "untested" * Unused code can be an indication of bugs/logical errors. By making the contributor aware of newly introduced unused code it gives him/her an opportunity to investigate if the unused code they introduce is malignant or benign :-) * Unused code is hard to spot for humans and is thus often missed during manual review * [YAGNI](https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it) Based on bitcoin#14312 to make linter job pass. Tree-SHA512: 4c581df7c34986e226e4ade479e0d3c549daf38f4a4dc4564b25564d63e773a1830ba55d1289c771b1fa325483e8855b82b56e61859fe8e4b7dfa54034b093b6

@MarcoFalke

ff94da7 tests: Make appveyor run with --usecli (practicalswift) db01839 test: Add missing call to skip_if_no_cli() (practicalswift) Pull request description: Add missing call to `skip_if_no_cli()` as suggested by @MarcoFalke in bitcoin#14365. Tree-SHA512: b0a2ddfad0f81cc9544f63c4e490fb983d833a47c23522549d1200ea6a8a132b2cd4bf0d66b862ef3a548d8471128b80aea3525fb5dec65221e23f32a8d46746

@MarcoFalke

ff94da7 tests: Make appveyor run with --usecli (practicalswift) db01839 test: Add missing call to skip_if_no_cli() (practicalswift) Pull request description: Add missing call to `skip_if_no_cli()` as suggested by @MarcoFalke in bitcoin#14365. Tree-SHA512: b0a2ddfad0f81cc9544f63c4e490fb983d833a47c23522549d1200ea6a8a132b2cd4bf0d66b862ef3a548d8471128b80aea3525fb5dec65221e23f32a8d46746

@MarcoFalke

ff94da7 tests: Make appveyor run with --usecli (practicalswift) db01839 test: Add missing call to skip_if_no_cli() (practicalswift) Pull request description: Add missing call to `skip_if_no_cli()` as suggested by @MarcoFalke in bitcoin#14365. Tree-SHA512: b0a2ddfad0f81cc9544f63c4e490fb983d833a47c23522549d1200ea6a8a132b2cd4bf0d66b862ef3a548d8471128b80aea3525fb5dec65221e23f32a8d46746

…Travis c82190c tests: Add Python dead code linter (vulture) (practicalswift) 590a57f tests: Remove unused testing code (practicalswift) Pull request description: Add Python dead code linter (`vulture`) to Travis. Rationale for allowing dead code only after explicit opt-in (via `--ignore-names`): * Less is more :-) * Unused code is by definition "untested" * Unused code can be an indication of bugs/logical errors. By making the contributor aware of newly introduced unused code it gives him/her an opportunity to investigate if the unused code they introduce is malignant or benign :-) * Unused code is hard to spot for humans and is thus often missed during manual review * [YAGNI](https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it) Based on bitcoin#14312 to make linter job pass. Tree-SHA512: 4c581df7c34986e226e4ade479e0d3c549daf38f4a4dc4564b25564d63e773a1830ba55d1289c771b1fa325483e8855b82b56e61859fe8e4b7dfa54034b093b6

practicalswift mentioned this pull request Oct 2, 2018

tests: Remove unused testing code #14312

Closed

practicalswift force-pushed the lint-python-dead-code branch from 69e9438 to ba76e96 Compare October 2, 2018 11:02

fanquake added the Tests label Oct 2, 2018

practicalswift force-pushed the lint-python-dead-code branch 2 times, most recently from 7d88eb5 to 6d3822d Compare October 2, 2018 11:51

promag reviewed Oct 2, 2018

View reviewed changes

maflcko reviewed Oct 3, 2018

View reviewed changes

test/functional/test_framework/test_framework.py Outdated Show resolved Hide resolved

practicalswift force-pushed the lint-python-dead-code branch from 6d3822d to acc7213 Compare October 3, 2018 08:07

practicalswift mentioned this pull request Oct 3, 2018

test: Add missing call to skip_if_no_cli() #14381

Merged

jb55 reviewed Oct 3, 2018

View reviewed changes

test/functional/test_framework/messages.py Outdated Show resolved Hide resolved

practicalswift force-pushed the lint-python-dead-code branch from acc7213 to 93f965c Compare October 4, 2018 07:53

practicalswift force-pushed the lint-python-dead-code branch from 93f965c to e3a08ac Compare October 4, 2018 20:54

practicalswift force-pushed the lint-python-dead-code branch from e3a08ac to 2348ebf Compare October 4, 2018 21:46

maflcko reviewed Oct 4, 2018

View reviewed changes

test/lint/lint-python-dead-code.sh Outdated Show resolved Hide resolved

practicalswift force-pushed the lint-python-dead-code branch from 2348ebf to c422cf2 Compare October 5, 2018 07:14

DrahtBot added the Needs rebase label Oct 20, 2018

DrahtBot removed the Needs rebase label Oct 21, 2018

practicalswift mentioned this pull request Oct 22, 2018

Enable flake8 rule E225 which checks for missing whitespace around op… #14534

Closed

DrahtBot mentioned this pull request Oct 27, 2018

lint: Run the CI lint stage on mac #13728

Merged

maflcko reviewed Nov 7, 2018

View reviewed changes

test/functional/test_framework/key.py Outdated Show resolved Hide resolved

tests: Remove unused testing code

590a57f

practicalswift force-pushed the lint-python-dead-code branch from 7c2dd72 to 5c6e977 Compare November 7, 2018 17:07

tests: Add Python dead code linter (vulture)

c82190c

practicalswift force-pushed the lint-python-dead-code branch from 5c6e977 to c82190c Compare November 7, 2018 17:09

maflcko merged commit c82190c into bitcoin:master Nov 7, 2018

practicalswift deleted the lint-python-dead-code branch April 10, 2021 19:36

pravblockc mentioned this pull request Aug 11, 2021

Backports v0.18: PR's 14530 and 14365 dashpay/dash#4331

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

tests: Add Python dead code linter (vulture) to Travis #14365

tests: Add Python dead code linter (vulture) to Travis #14365

Uh oh!

Conversation

practicalswift commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Oct 2, 2018

Uh oh!

Uh oh!

practicalswift commented Oct 3, 2018

Uh oh!

Uh oh!

promag commented Oct 4, 2018

Uh oh!

practicalswift commented Oct 4, 2018

Uh oh!

promag commented Oct 4, 2018

Uh oh!

practicalswift commented Oct 4, 2018

Uh oh!

Uh oh!

promag commented Oct 4, 2018

Uh oh!

practicalswift commented Oct 5, 2018

Uh oh!

ken2812221 commented Oct 6, 2018

Uh oh!

jb55 commented Oct 8, 2018 via email

Uh oh!

conscott commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

maflcko commented Nov 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

practicalswift commented Oct 2, 2018 •

edited

Loading

practicalswift commented Oct 2, 2018 •

edited

Loading

conscott commented Oct 12, 2018 •

edited

Loading

practicalswift commented Oct 16, 2018 •

edited

Loading

DrahtBot commented Oct 27, 2018 •

edited

Loading