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

[jsonrpsee types]: unify a couple of types + more tests #389

Merged
merged 2 commits into from
Jun 24, 2021

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Jun 21, 2021

This PR adds some additional tests to close #276 (remaining is the ignored tests for RpcParams that are untyped but as long we provide RpcParams::parse for it, it should be fine)

Then I was annoyed of writing so much tests so I unified JsonRpcNotifResponse and JsonRpcNotification and JsonRpcNotificationParamsAlloc and JsonRpcNotificationParams`.

Thus, I replaced RawValue with T it will require allocations when deserializing but the benefit is that we don't have convert values to RawValue when sending stuff via the SubscriptionSink

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

LGTM

types/src/v2/params.rs Outdated Show resolved Hide resolved
types/src/v2/params.rs Outdated Show resolved Hide resolved
})
.expect("valid json infallible; qed");
let msg =
self.build_message(&SubscriptionClosedError::from(close_reason)).expect("valid json infallible; qed");
Copy link
Member

Choose a reason for hiding this comment

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

Whats qed?

Copy link
Member Author

@niklasad1 niklasad1 Jun 22, 2021

Choose a reason for hiding this comment

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

https://en.wikipedia.org/wiki/Q.E.D.

basically, a hand-wavy proof that a given unwrap/expect is infallible otherwise it would crash the thread that executed the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's latin, old-europe snobbish way of saying "tada!"

Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

Few questions and one tiny nit pick, but overall lgtm.

@niklasad1 niklasad1 merged commit 13ea5b2 into master Jun 24, 2021
@niklasad1 niklasad1 deleted the na-jsonrpsee-types-fix-complete-tests branch June 24, 2021 08:33
dvdplm added a commit that referenced this pull request Jul 6, 2021
* master: (21 commits)
  New proc macro (#387)
  Streaming RpcParams parsing (#401)
  Set allowed Host header values (#399)
  Synchronization-less async connections in ws-server (#388)
  [ws server]: terminate already established connection(s) when the server is stopped (#396)
  feat: customizable JSON-RPC error codes via new enum variant on `CallErrror` (#394)
  [ci]: test each individual crate's manifest (#392)
  Add a way to stop servers (#386)
  [jsonrpsee types]: unify a couple of types + more tests (#389)
  Update roadmap link in readme (#390)
  Cross-origin protection (#375)
  Method aliases + RpcModule: Clone (#383)
  Use criterion's async bencher (#385)
  Async/subscription benches (#372)
  send text (#374)
  Fix link to ws server in README.md (#373)
  Concat -> simple push (#370)
  Add missing `rt` feature (#369)
  Release prep for v0.2 (#368)
  chore(scripts): publish script (#354)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[types]: jsonrpc_v2 types write serialize/deserialize tests.
3 participants