Crate restructuring #590

maciejhirsz · 2021-12-06T08:38:48Z

I need to start making smaller PRs. This one is pretty straight forward, removed the v2 module and moved things around to hopefully sensible locations.

Rationale: We added the v2 module when doing some internal refactoring that ultimately lead to different versions of types with similar names (Request most notably), but have since unified the apis to use same types, and with the "v1" absent there is no need for the module to exist.

Edit: see comment for ongoing changes.

types/src/lib.rs

niklasad1

👍

dvdplm · 2021-12-06T14:09:21Z

@maciejhirsz The PR description is a bit lacking here. Can you elaborate on why removing the v2 module is good? Not saying I disagree, but it is there for a reason (separate spec-mandated types from the rest) so it'd be good to write down why that reason is invalid/superseded.

utils/src/server/helpers.rs

maciejhirsz · 2021-12-09T13:04:18Z

@dvdplm Should be all tidy now + rationale, thoughts on having a linter for forcing braces on all the imports?

Edit: also @niklasad1 opinions here? :)

niklasad1 · 2021-12-09T13:30:04Z

Rationale: We added the v2 module when doing some internal refactoring that ultimately lead to different versions of types with similar names (Request most notably), but have since unified the apis to use same types, and with the "v1" absent there is no need for the module to exist.

..., I thought we used v2 to indicate that these types are JSON-RPC v2 spec related types and not just common types after we removed the JSON-RPC spec related from the jsonrpc crate (way back). I like this change because it's a bit annoying to have to write jsonrpsee_types::v2::Response instead of jsonrpsee_types::Response...

thoughts on having a linter for forcing braces on all the imports

I don't like the braces for single imports if that's what you are suggesting but.... feel free to convince me :)
Then we need some special rustfmt rule...

maciejhirsz · 2021-12-09T14:06:21Z

..., I thought we used v2 to indicate that these types are JSON-RPC v2 spec related types (...)

We did? I thought it was just to keep stuff separate and it always bugged me. Given that 2.0 is the only version we support, and I don't think it's likely to change in the future I also don't see why we would need that differentiation.

I don't like the braces for single imports if that's what you are suggesting but.... feel free to convince me :)
Then we need some special rustfmt rule...

use foo::{a, b, c};
use foo::{d, e, f};

Or

use foo::{
    a, b, c,
    d, e, f,
};

(these names are short enough to fit in a single line, but you get the idea)

niklasad1 · 2021-12-09T14:25:10Z

We did? I thought it was just to keep stuff separate and it always bugged me. Given that 2.0 is the only version we support, and I don't think it's likely to change in the future I also don't see why we would need that differentiation.

I think it was removed in https://github.com/paritytech/jsonrpsee/pull/269/files, maybe it's not needed but made it clear for which part of types that were just shared/common types in the repo and v2 - types that were strictly JSON-RPC specification related. However, I like it :)

(these names are short enough to fit in a single line, but you get the idea)

I don't care that much about that but I like 👇 better

use foo::{a, b, c};
use foo::{d, e, f};

maciejhirsz · 2021-12-09T14:27:07Z

I don't care that much about that but I like point_down better

Yeah, that's my preference too, I can revert some of the imports here.

dvdplm · 2021-12-10T11:54:43Z

We did? I thought it was just to keep stuff separate and it always bugged me. Given that 2.0 is the only version we support, and I don't think it's likely to change in the future I also don't see why we would need that differentiation.

It was never so much about versioning as it was about separating spec mandated types from the rest. The types from the spec must be kept intact forever while other public types can change with a breaking release. I think that is a worthy motivation for keeping things separate, and v2 seemed like a decent enough name for it.
However, given that wasn't clear to you @maciejhirsz I bet it isn't to many others, so we should perhaps consider a different name, maybe types::spec::StuffFromTheSpec?

maciejhirsz · 2021-12-10T13:49:22Z

The types from the spec must be kept intact forever while other public types can change with a breaking release.

I don't think that's true, we can easily make a breaking change to a spec type by changing stuff like String to Cow<'a, str>, or getting rid of the TwoPointZero type and just building a manual Serialize impl on request etc., without breaking spec compatibility.

dvdplm · 2021-12-12T21:37:53Z

I don't think that's true, we can easily make a breaking change to a spec type by changing stuff like String to Cow<'a, str>, or getting rid of the TwoPointZero type and just building a manual Serialize impl on request etc., without breaking spec compatibility.

You are correct, but you're missing the point: these types do require some extra care outside of the normal semver compatibility narrative. Question is: is this something that is worth signalling by keeping them in their own namespace? I think so, and that's what lies behind the v2 module.
I'm not opposed to rename the module and I'm willing to listen to arguments against keeping them separate from the rest but so far I remain unconvinced.

dvdplm · 2021-12-14T11:02:37Z

After discussion here's the plan:

Rename jsonrpsee_utils to jsonrpsee_core
Rename jsonrpsee_types::RpcError to jsonrpsee_types::ErrorResponse.
Move everything that's not a JSON-RCP transport type from jsonrpsee_types to jsonrpsee_core. This includes:
3.1 Middleware and traits.
3.2 Serialize, DeserializeOwned, beef::Cow, to_json_value, to_json_raw_value re-exports.
3.3 All the error stuff that's not ErrorResponse, enums and other internal utils.

dvdplm · 2021-12-14T16:50:49Z

ws-client/src/client.rs

@@ -590,7 +593,7 @@ async fn background_task(
 					}
 				}
 				// Error response


Clippy is angry about line 195

core/src/error.rs

core/src/server/mod.rs

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

niklasad1 · 2021-12-16T17:13:16Z

core/src/lib.rs

+}
+
+pub use beef::Cow;
+pub use serde::{de::DeserializeOwned, Serialize};


maybe we should re-export Deserialize here too..

Mayhaps, we are also super inconsistent about reexports, sometimes we put them in __reexports module, and sometimes not, I'd leave that for a future PR.

core/src/traits.rs

proc-macros/src/rpc_macro.rs

niklasad1 · 2021-12-16T17:21:04Z

core/Cargo.toml

-hyper = { version = "0.14.10", default-features = false, features = ["stream"], optional = true }
-jsonrpsee-types = { path = "../types", version = "0.6.0", optional = true }
+hyper = { version = "0.14.10", default-features = false, features = ["stream"] }
+jsonrpsee-types = { path = "../types", version = "0.6.0" }


this crate is now what the v2 was?

niklasad1

LGTM, a few small questions....

This makes the repo structure more clear all good.

TarikGul

lgtm, It generally looks way cleaner to me (ie. the imports are way easier to follow).

The Macro export for rpc_params in core/src/client.rs took me a hot second to understand because github formatted it very weird, but overall 👍 from me.

maciejhirsz added 2 commits December 5, 2021 22:56

Nuke V2

cb3e37d

fmt

09ceb90

maciejhirsz requested review from dvdplm and niklasad1 December 6, 2021 08:38

maciejhirsz requested a review from a team as a code owner December 6, 2021 08:38

niklasad1 reviewed Dec 6, 2021

View reviewed changes

types/src/lib.rs Outdated Show resolved Hide resolved

niklasad1 approved these changes Dec 6, 2021

View reviewed changes

dvdplm reviewed Dec 6, 2021

View reviewed changes

utils/src/server/helpers.rs Outdated Show resolved Hide resolved

maciejhirsz added 5 commits December 9, 2021 12:42

Formatting and imports

714ce5d

Merge branch 'master' of github.com:paritytech/jsonrpsee into mh-cleanup

4d79729

Updated benches

22dd3a7

Fix doc comment link

6bb0224

Brace imports in ws-server

bc22ac2

maciejhirsz added 6 commits December 10, 2021 12:21

Reworking imports

392daa7

Merge branch 'master' of github.com:paritytech/jsonrpsee into mh-cleanup

a9c37fa

std first

57bdf5a

fmt

e24f12d

std on top

e59732f

Update to match changed line numbers

4f32c88

Merge branch 'master' of github.com:paritytech/jsonrpsee into mh-cleanup

3ec6e62

maciejhirsz added 4 commits December 14, 2021 11:25

Rename jsonrpsee_utils -> jsonrpsee_core

f2f5315

Migrating things types -> core

e29bdec

RpcError -> ErrorResponse

faa0da1

Merge types::client into core::client

a9567f0

maciejhirsz changed the title ~~Nuke types::v2~~ [WIP] Crate restructuring Dec 14, 2021

dvdplm reviewed Dec 14, 2021

View reviewed changes

ws-client/src/client.rs

@@ -590,7 +593,7 @@ async fn background_task(

}

}

// Error response

Copy link

Contributor

dvdplm Dec 14, 2021

Choose a reason for hiding this comment

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

Clippy is angry about line 195

niklasad1 self-requested a review December 14, 2021 17:04

Continued move types -> core

f914dca

TarikGul reviewed Dec 15, 2021

View reviewed changes

core/src/error.rs Show resolved Hide resolved

maciejhirsz added 4 commits December 15, 2021 16:30

Removing features to make checks pass

6c15874

Move rpc_module tests to tests crate

7ed2316

Merge branch 'master' of github.com:paritytech/jsonrpsee into mh-cleanup

1d09ee4

Fixed doc comment links

dc4ab02

niklasad1 reviewed Dec 16, 2021

View reviewed changes

core/src/server/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Dec 16, 2021

View reviewed changes

core/src/server/mod.rs Outdated Show resolved Hide resolved

maciejhirsz and others added 2 commits December 16, 2021 15:26

Add futures-util dependency for client

394a3c2

Remove dead code

c38ec76

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

maciejhirsz changed the title ~~[WIP] Crate restructuring~~ Crate restructuring Dec 16, 2021

fmt

e93c2d9

niklasad1 reviewed Dec 16, 2021

View reviewed changes

core/src/traits.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Dec 16, 2021

View reviewed changes

proc-macros/src/rpc_macro.rs Show resolved Hide resolved

niklasad1 reviewed Dec 16, 2021

View reviewed changes

niklasad1 approved these changes Dec 16, 2021

View reviewed changes

maciejhirsz requested a review from a team December 17, 2021 08:45

TarikGul approved these changes Dec 17, 2021

View reviewed changes

maciejhirsz added 2 commits December 17, 2021 14:23

Feature gate the Client trait

51eec24

Move Client traits to client module

9a88c70

maciejhirsz merged commit e159c44 into master Dec 17, 2021

maciejhirsz deleted the mh-cleanup branch December 17, 2021 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crate restructuring #590

Crate restructuring #590

maciejhirsz commented Dec 6, 2021 •

edited

Loading

niklasad1 left a comment

dvdplm commented Dec 6, 2021

maciejhirsz commented Dec 9, 2021 •

edited

Loading

niklasad1 commented Dec 9, 2021

maciejhirsz commented Dec 9, 2021

niklasad1 commented Dec 9, 2021 •

edited

Loading

maciejhirsz commented Dec 9, 2021

dvdplm commented Dec 10, 2021

maciejhirsz commented Dec 10, 2021

dvdplm commented Dec 12, 2021

dvdplm commented Dec 14, 2021

dvdplm Dec 14, 2021

niklasad1 Dec 16, 2021

maciejhirsz Dec 17, 2021

niklasad1 Dec 16, 2021

maciejhirsz Dec 17, 2021

niklasad1 left a comment

TarikGul left a comment

Crate restructuring #590

Crate restructuring #590

Conversation

maciejhirsz commented Dec 6, 2021 • edited Loading

niklasad1 left a comment

Choose a reason for hiding this comment

dvdplm commented Dec 6, 2021

maciejhirsz commented Dec 9, 2021 • edited Loading

niklasad1 commented Dec 9, 2021

maciejhirsz commented Dec 9, 2021

niklasad1 commented Dec 9, 2021 • edited Loading

maciejhirsz commented Dec 9, 2021

dvdplm commented Dec 10, 2021

maciejhirsz commented Dec 10, 2021

dvdplm commented Dec 12, 2021

dvdplm commented Dec 14, 2021

dvdplm Dec 14, 2021

Choose a reason for hiding this comment

niklasad1 Dec 16, 2021

Choose a reason for hiding this comment

maciejhirsz Dec 17, 2021

Choose a reason for hiding this comment

niklasad1 Dec 16, 2021

Choose a reason for hiding this comment

maciejhirsz Dec 17, 2021

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

TarikGul left a comment

Choose a reason for hiding this comment

maciejhirsz commented Dec 6, 2021 •

edited

Loading

maciejhirsz commented Dec 9, 2021 •

edited

Loading

niklasad1 commented Dec 9, 2021 •

edited

Loading