Skip to content
This repository has been archived by the owner on Sep 4, 2024. It is now read-only.

Accept also None or Objects as params #108

Merged
merged 5 commits into from
Nov 18, 2023

Conversation

RCasatta
Copy link
Contributor

@RCasatta RCasatta commented Nov 9, 2023

according to the jsonrpc spec params can be missing, or an object too.

close #107

Breaking change

@apoelstra
Copy link
Owner

concept ACK. But looks like certain feature combos are broken (see CI)

@RCasatta
Copy link
Contributor Author

RCasatta commented Nov 9, 2023

I thought after the first approval the other times the CI would start automatically, but it's not the case, I hope to have "blindly" fixed it.

@RCasatta
Copy link
Contributor Author

I think to have tested all the cases launched with the CI locally, sorry for the back and forth

@apoelstra
Copy link
Owner

I thought after the first approval the other times the CI would start automatically, but it's not the case, I hope to have "blindly" fixed it.

All good. I blame Github, not you :).

Hit CI. Let's see how it goes.

Copy link
Owner

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 2c3f462

@apoelstra
Copy link
Owner

@RCasatta can you add a unit test that demonstrates how to provide a list, and maybe a CHANGELOG entry explaining this? This will be a breaking change which will be particularly annoying because the RawValue API is so annoying.

according to the jsonrpc spec params can be missing, or an object too
@RCasatta RCasatta force-pushed the accept_object3 branch 2 times, most recently from c8986b8 to 178ee93 Compare November 14, 2023 09:32
@RCasatta
Copy link
Contributor Author

I had to force push because there were a couple of reference of &[] not changed and not tested in CI (the one in the Readme and the one in integration_test/src/main.rs)

Moreover, I added:

  • the unit test commit
  • a call getblockhash in the integration test with both list and named configuration (not launched in CI though)
  • update the changelog

Copy link
Collaborator

@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 178ee93

@@ -1,3 +1,8 @@
# 0.17.0 - 2023-06-29
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Perhaps use "unreleased" so we don't forget to update the header when doing the release.

Suggested change
# 0.17.0 - 2023-06-29
# unreleased

@RCasatta RCasatta requested a review from apoelstra November 16, 2023 14:27
@apoelstra
Copy link
Owner

Looks like some clippy lints are causing CI to fail.

@RCasatta
Copy link
Contributor Author

Sorry again, should be fixed

Copy link
Collaborator

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

@apoelstra
Copy link
Owner

Previously you had to provide a list with each thing wrapped in arg(), which was bad enough ... but now it looks like you need to wrap your lists in Some(to_raw_value(&json!(actual_thing)).unwrap()) which is way worse.

I think we need a better API here.

@RCasatta
Copy link
Contributor Author

You don't need the json! call when you have a struct with serde::Serialize

@apoelstra
Copy link
Owner

@RCasatta ah, I gotcha. So in your list unit test, where you write json!([0]) it's actually fine to just do to_raw_value(&[0]) (I tested this and it worked).

Ok, I'm happy then.

Copy link
Owner

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 2c3f462

@apoelstra apoelstra merged commit 9e7a0ae into apoelstra:master Nov 18, 2023
9 checks passed
@RCasatta RCasatta mentioned this pull request Dec 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support params as Object
3 participants