Skip to content

Conversation

GideonBature
Copy link
Contributor

@GideonBature GideonBature commented May 28, 2025

The JSON-RPC method disconnectnode does not return anything. We want to test this to catch any changes in behavior in future Core versions.

This PR adds a client function that errors if the return value is anything other than null, along with an integration test that calls this function.

Ref: #116

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Looks good except the test name. Perhaps since you are using the same PR description for each of these PRs you do we could make it more terse and more meaningful. Something like

'JSONRPC method foo does not return anything, we would like to test this in case a later version of Core changes this behaviour. Add a function to the client that errors if anything but null is returned then add an integration test that calls this new function.'

@GideonBature
Copy link
Contributor Author

GideonBature commented May 29, 2025

Looks good except the test name. Perhaps since you are using the same PR description for each of these PRs you do we could make it more terse and more meaningful. Something like

'JSONRPC method foo does not return anything, we would like to test this in case a later version of Core changes this behaviour. Add a function to the client that errors if anything but null is returned then add an integration test that calls this new function.'

This is better... Will adopt this. Thank you!

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 359ac38

@tcharding tcharding merged commit 30a9511 into rust-bitcoin:master May 29, 2025
29 checks passed
@GideonBature GideonBature deleted the disconnectnode branch May 31, 2025 08:09
blaze-smith470pm added a commit to blaze-smith470pm/corepc that referenced this pull request Sep 26, 2025
359ac383f28c579d5ed79ded0bba0a38cb765f0c Implement disconnectnode method and test (GideonBature)

Pull request description:

  Going by the conversations with tcharding for PR [#116 ](rust-bitcoin/corepc#116) on implementing one method at a time for easier review: This is the disconnectnode method which is a specific type that returns a (json null). Once this is approved, I’ll proceed with the next one.

ACKs for top commit:
  tcharding:
    ACK 359ac383f28c579d5ed79ded0bba0a38cb765f0c

Tree-SHA512: 3b16431e732e81edea1f290cf6436ae5ad0e72a591546b09e69eb47092c95ccef5e07be8908f20d32fb87a77cf623caf07eee0e96b96bc2fab7240af23e4286a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants