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

test: make the node param explicit in init_wallet() #23316

Merged
merged 1 commit into from
Oct 20, 2021

Conversation

lsilva01
Copy link
Contributor

This PR changes the definition of def init_wallet(self, i) to def init_wallet(self, *, node) to make the node parameter explicit, as suggested in #22794 (comment) .

@fanquake fanquake added the Tests label Oct 20, 2021
@lsilva01 lsilva01 changed the title Make the node param explicit in init_wallet() test: make the node param explicit in init_wallet() Oct 20, 2021
@brunoerg
Copy link
Contributor

Concept ACK

Copy link
Contributor

@stratospher stratospher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested ACK 7b3c9e4.

This is a nice change! The function call self.init_wallet(node=0) is more understandable compared to self.init_wallet(0). This call format is also consistent with other functions from the codebase like test_cltv_info and test_dersig_info which are defined in a similar manner. Verified that there aren’t other instances making the init_wallet() call.

@maflcko maflcko merged commit 1435161 into bitcoin:master Oct 20, 2021
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posthumous ACK

self.init_wallet(2)
self.init_wallet(node=0)
self.init_wallet(node=1)
self.init_wallet(node=2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

     def init_three(self):
-        self.init_wallet(node=0)
-        self.init_wallet(node=1)
-        self.init_wallet(node=2)
+        for n in range(3):
+            self.init_wallet(node=n)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the change in #23327.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 20, 2021
7b3c9e4 Make explicit the node param in init_wallet() (lsilva01)

Pull request description:

  This PR changes the definition of `def init_wallet(self, i)` to `def init_wallet(self, *, node)` to make the node parameter explicit, as suggested in bitcoin#22794 (comment) .

ACKs for top commit:
  stratospher:
    tested ACK 7b3c9e4.

Tree-SHA512: 2ef036f4c2110b2f7dc893dc6eea8faa0a18edd7f8f59b25460a6c544df7238175ddd6a0d766e2bb206326b1c9afc84238c75613a0f01eeda89a8ccb7d86a4f1
@lsilva01 lsilva01 deleted the init_wallet_node_param branch October 20, 2021 23:42
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants