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

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

Merged
merged 2 commits into from Nov 7, 2018

Conversation

Projects
None yet
10 participants
@practicalswift
Copy link
Member

commented Oct 2, 2018

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 practicalswift force-pushed the practicalswift:lint-python-dead-code branch Oct 2, 2018

@fanquake fanquake added the Tests label Oct 2, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2018

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 :-)

@practicalswift practicalswift force-pushed the practicalswift:lint-python-dead-code branch 2 times, most recently Oct 2, 2018

@promag
Copy link
Member

left a comment

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

I suggest closing #14312.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2018

@promag Thanks for reviewing. Good idea! Done.

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

@practicalswift practicalswift force-pushed the practicalswift:lint-python-dead-code branch Oct 3, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2018

@MarcoFalke Updated. Please re-review :-)

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Oct 4, 2018

Merge bitcoin#14381: test: Add missing call to skip_if_no_cli()
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

@practicalswift practicalswift force-pushed the practicalswift:lint-python-dead-code branch Oct 4, 2018

@promag

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

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

@practicalswift practicalswift force-pushed the practicalswift:lint-python-dead-code branch Oct 4, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

@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

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

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 practicalswift force-pushed the practicalswift:lint-python-dead-code branch Oct 4, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

@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.

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

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

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

You have.

@practicalswift practicalswift force-pushed the practicalswift:lint-python-dead-code branch Oct 5, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2018

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

You have.

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

@ken2812221

This comment has been minimized.

Copy link
Member

commented Oct 6, 2018

utACK c422cf2b50b08b489172de28f69083784c35a455

1 similar comment
@jb55

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

@conscott

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

utACk c422cf2b50b08b489172de28f69083784c35a455 - vulture is great!

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

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 :-)

@practicalswift practicalswift force-pushed the practicalswift:lint-python-dead-code branch Oct 20, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2018

Rebased! :-)

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2018

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.

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

@practicalswift practicalswift force-pushed the practicalswift:lint-python-dead-code branch Nov 7, 2018

@practicalswift practicalswift force-pushed the practicalswift:lint-python-dead-code branch to c82190c Nov 7, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

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

@MarcoFalke MarcoFalke merged commit c82190c into bitcoin:master Nov 7, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

MarcoFalke added a commit that referenced this pull request Nov 7, 2018

Merge #14365: tests: Add Python dead code linter (vulture) to 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 #14312 to make linter job pass.

Tree-SHA512: 4c581df7c34986e226e4ade479e0d3c549daf38f4a4dc4564b25564d63e773a1830ba55d1289c771b1fa325483e8855b82b56e61859fe8e4b7dfa54034b093b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.