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

Rename and reorg types #462

Merged
merged 10 commits into from
Sep 15, 2021
Merged

Rename and reorg types #462

merged 10 commits into from
Sep 15, 2021

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Sep 11, 2021

⚠️ Breaking change ⚠️

Closes #457.

The changes here mostly removes the JsonRpc prefix from the types that come from the spec. I have kept a prefix for some types with a very common stdlib version such as Result and Error. I also changed the serializable versions to consistently use the *Ser postfix (not loving this convention but have no better suggestion).

I'm tempted to make the jsonrpsee-types paths more shallow, i.e. re-export all the modules under v2, so that instead of doing jsonrpsee-types::v2::error::RpcError we'd have jsonrpsee-types::v2::RpcError; instead of jsonrpsee-types::v2::response::Response we'd have jsonrpsee-types::v2::Response etc. Thoughts?

@dvdplm dvdplm self-assigned this Sep 11, 2021
@dvdplm dvdplm marked this pull request as ready for review September 11, 2021 10:22
@maciejhirsz
Copy link
Contributor

How about CallParams instead of ParamsSer? Or, if we want to be slightly confusing but pedantically correct: Arguments.

@dvdplm
Copy link
Contributor Author

dvdplm commented Sep 14, 2021

How about CallParams instead of ParamsSer? Or, if we want to be slightly confusing but pedantically correct: Arguments.

GH threw away my reply to this, grr.

I like Arguments quite a lot, but I don't think it's wise to move away from "params" because the spec uses "parameter" throughout and I don't think it's good to ask users/readers to map from one to the other.

The downside of going with CallParams is that it doesn't help with the other *Ser types: RequestSer and NotificationSer. Using a post-fix has the advantage of clearly communicating what is peculiar about all three of them.

:/

@niklasad1
Copy link
Member

I'm tempted to make the jsonrpsee-types paths more shallow, i.e. re-export all the modules under v2, so that instead of doing jsonrpsee-types::v2::error::RpcError we'd have jsonrpsee-types::v2::RpcError; instead of jsonrpsee-types::v2::response::Response we'd have jsonrpsee-types::v2::Response etc. Thoughts?

Sounds good to me

ws-client/src/helpers.rs Outdated Show resolved Hide resolved
@niklasad1
Copy link
Member

niklasad1 commented Sep 15, 2021

How about CallParams instead of ParamsSer? Or, if we want to be slightly confusing but pedantically correct: Arguments.

We have used Params because the spec states that. I think CallParams is slightly confusing because when the server receives the params it's still a call?! At least ParamsSer is super obvious too me even if it's a bit "verbose"

Arguments could work though....

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM, some minor comments to address

In an followup PR, we could flatten the v2 mod and remove the child modules request, response, params.

@dvdplm
Copy link
Contributor Author

dvdplm commented Sep 15, 2021

In an followup PR, we could flatten the v2 mod and remove the child modules request, response, params.

#468

Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

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

Aside form my initial bikeshed it looks good to me :)

@dvdplm dvdplm merged commit 635142e into master Sep 15, 2021
@dvdplm dvdplm deleted the dp-rename-and-reorg-types branch September 15, 2021 14:13
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.

Rename and reorganise types a bit
3 participants