apoelstra / rust-jsonrpc

Rust JSONRPC library
Creative Commons Zero v1.0 Universal
158 stars 62 forks source link

Accept also None or Objects as params #108

Closed RCasatta closed 10 months ago

RCasatta commented 11 months ago

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

close #107

Breaking change

apoelstra commented 11 months ago

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

RCasatta commented 11 months ago

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 commented 10 months ago

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

apoelstra commented 10 months ago

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.

apoelstra commented 10 months ago

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

RCasatta commented 10 months ago

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:

apoelstra commented 10 months ago

Looks like some clippy lints are causing CI to fail.

RCasatta commented 10 months ago

Sorry again, should be fixed

apoelstra commented 10 months ago

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 commented 10 months ago

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

apoelstra commented 10 months ago

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