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] tidy up wallet_importmulti.py #15108

Merged
merged 2 commits into from Jan 9, 2019

Conversation

Projects
None yet
8 participants
@amitiuttarwar
Copy link
Contributor

commented Jan 5, 2019

Cherry picks un-merged commits from #14952, which "fixes review comments from @ryanofsky here: #14886 (review)"

jnewbery added some commits Dec 13, 2018

[tests] tidy up wallet_importmulti.py
Fixes review comments from PR 14886.
[tests] move wallet util functions to wallet_util.py
Adds a new wallet_util.py module and moves generic helper functions
there:

- get_key
- get_multisig
- test_address
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15032 (Nit fixes for PR 14565 by MeshCollider)

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.

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

Concept ACK, will review later.

@promag
Copy link
Member

left a comment

@amitiuttarwar side note, looks like you haven't added your email to github?

@@ -0,0 +1,99 @@
#!/usr/bin/env python3
# Copyright (c) 2018 The Bitcoin Core developers

This comment has been minimized.

Copy link
@promag

promag Jan 7, 2019

Member

2019

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jan 7, 2019

Member

This was authored in 2018 (according to the git author date), so I guess it is fine.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

utACK 2d5f1ea

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Concept ACK

@jnewbery
Copy link
Member

left a comment

Looks great. Thanks for picking this up @amitiuttarwar!

There were a couple of review comments from @practicalswift in #14952 which weren't addressed in the base PR #14565, so you could fix them in this PR if you wanted:

  • wallet_importmulti.py L52 (observed_warnings = result[0]['warnings'] is underindented)
  • the default argument for warnings in test_importmulti() should be None, not [] (see 'Mutable Default Arguments' in https://docs.python-guide.org/writing/gotchas/)

I've also added one small nit of my own.

tested ACK 2d5f1ea

"timestamp": "now",
"keys": [wrong_privkey]},
True,
success=True,

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jan 7, 2019

Member

micronit: this line (and the following) is overindented. Remove one leading space (so success is aligned with the opening parens of the function call, not the the opening braces of the object above)

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

tACK 2d5f1ea

Fixing nits is nice, but adding tests to #14491 will be easier once this is merged (cc @meshcollider).

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

I am merging this, because it has some review and the nits are only about whitespace in touched lines or additional cleanup in unrelated lines. Please fix them up separately if anyone feels like it.

@MarcoFalke MarcoFalke merged commit 2d5f1ea into bitcoin:master Jan 9, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Jan 9, 2019

Merge #15108: [tests] tidy up wallet_importmulti.py
2d5f1ea [tests] move wallet util functions to wallet_util.py (John Newbery)
6be64ef [tests] tidy up wallet_importmulti.py (John Newbery)

Pull request description:

  Cherry picks un-merged commits from #14952, which "fixes review comments from @ryanofsky here: #14886 (review)"

Tree-SHA512: 5f389196b0140d013a533d500f1812786a3a5cfb65980e13eaeacc459fddb55f43d05da3ab5e7cc8c997f26c0b667eed081ab6de2d125e631c70a7dd4c06e350

@amitiuttarwar amitiuttarwar deleted the amitiuttarwar:14952_rebase branch Jan 14, 2019

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.