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

lint: Check that all wallet args are hidden #15920

Merged
merged 1 commit into from Apr 29, 2019

Conversation

Projects
None yet
4 participants
@MarcoFalke
Copy link
Member

commented Apr 29, 2019

Can be tested by calling git revert 765d5890be and then running the script

@fanquake fanquake added the Tests label Apr 29, 2019

@fanquake

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

utACK fac174e

$ merge 15920
[pull/15920/local-merge d4663cc6a] Merge #15920: lint: Check that all wallet args are hidden
 Date: Mon Apr 29 20:50:41 2019 +0800
#15920 lint: Check that all wallet args are hidden into master
* fac174e2d lint: Check that all wallet args are hidden (MarcoFalke) (pull/15920/head)

Dropping you on a shell so you can try building/testing the merged source.
Run 'git diff HEAD~' to show the changes being merged.
Type 'exit' when done.

bash-3.2$ git revert 765d5890be
[pull/15920/local-merge c8412b746] Revert "Bugfix: dummywallet: Add -ignorepartialspends to list of ignored wallet options"
 1 file changed, 1 deletion(-)

bash-3.2$ test/lint/check-doc.py
Args used        : 175
Args documented  : 183
Args undocumented: 0
set()
Args unknown     : 8
{'-zmqpubhashtxhwm', '-zmqpubrawtxhwm', '-zmqpubhashtx', '-zmqpubrawblock', '-zmqpubrawblockhwm', '-zmqpubrawtx', '-zmqpubhashblockhwm', '-zmqpubhashblock'}
Traceback (most recent call last):
  File "test/lint/check-doc.py", line 66, in <module>
    main()
  File "test/lint/check-doc.py", line 62, in main
    lint_missing_hidden_wallet_args()
  File "test/lint/check-doc.py", line 57, in lint_missing_hidden_wallet_args
    assert 0, "Please add {} to the hidden args in DummyWalletInit::AddWalletOptions".format(hidden_missing)
AssertionError: Please add {'-avoidpartialspends'} to the hidden args in DummyWalletInit::AddWalletOptions

bash-3.2$ git revert c8412b746
[pull/15920/local-merge c40e287ad] Revert "Revert "Bugfix: dummywallet: Add -ignorepartialspends to list of ignored wallet options""
 1 file changed, 1 insertion(+)

bash-3.2$ test/lint/check-doc.py
Args used        : 175
Args documented  : 183
Args undocumented: 0
set()
Args unknown     : 8
{'-zmqpubhashblockhwm', '-zmqpubrawtxhwm', '-zmqpubhashblock', '-zmqpubhashtx', '-zmqpubrawblockhwm', '-zmqpubrawblock', '-zmqpubrawtx', '-zmqpubhashtxhwm'}
@practicalswift

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

tACK fac174e

Very nice to have a linter making sure that reviewers never have to think about this ever again :-)

@fanquake Out of curiosity: what merge alias/script (merge 15920 in your the shell session you posted) do you use to easily check out the PR locally? I've written such an alias/script myself for my needs but I'd rather use a less hacky packaged alias/script if available :-)

@fanquake

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@practicalswift It's the github-merge.py script in contrib/devtools.

@MarcoFalke MarcoFalke merged commit fac174e into bitcoin:master Apr 29, 2019

1 of 2 checks passed

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

MarcoFalke added a commit that referenced this pull request Apr 29, 2019

Merge #15920: lint: Check that all wallet args are hidden
fac174e lint: Check that all wallet args are hidden (MarcoFalke)

Pull request description:

  Can be tested by calling `git revert 765d589` and then running the script

ACKs for commit fac174:
  fanquake:
    utACK fac174e
  practicalswift:
    tACK fac174e

Tree-SHA512: f7d40dc3d9f471c0cf77bc2746c1ef09b9df093b24508e72bfc50114c338e5dcb4a17741cf97566aeddc6d608f13e4eb1c986ae9935cebad1d589495ac16e0b2

@MarcoFalke MarcoFalke deleted the MarcoFalke:1904-lintWalletArgsHidden branch Apr 29, 2019

@luke-jr

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@practicalswift Linters are NOT a substitute for proper review!

@practicalswift

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@luke-jr No, obviously not. Linters complement proper human review. Ideally they catch errors before any human review takes place.

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.