Conversation
tomusdrw
changed the title
local client to support metadata (Kudos Seun)
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
A PR description would be good.
This adds a new type parameter; isn't that a breaking change?
|
|
||
| /// Connects to a `Deref<Target = MetaIoHandler<Metadata + Default>`. | ||
| pub fn connect<TClient, THandler, TMetadata>(handler: THandler) -> (TClient, impl Future<Item = (), Error = RpcError>) | ||
| pub fn connect<TClient, THandler, TMetadata, TMiddleware>(handler: THandler) -> (TClient, impl Future<Item = (), Error = RpcError>) |
There was a problem hiding this comment.
As @dvdplm noticed it's a breaking change (so would require bumping to 16), perhaps better to add a new set of methods like connect_with_middleware or set the default type somehow?
Usage of middlewares is not super common, so I think it's better to keep the default connect stuff simple.
tomusdrw
changed the title
local client to support metadata (Kudos Seun)local client to support middleware (Kudos Seun)
niklasad1
left a comment
niklasad1
left a comment
There was a problem hiding this comment.
Is it reasonable to merge with master instead after the breaking changes? I think there is some unreleased stuff there (futures v0.3) IIRC.
|
any luck this can be merged and released today? |
|
@seunlanlege Please see #589 (comment) |
|
Weird I thought it could be released under 15.1? Either way, bumping to 16 feels extreme. Adding a new method, |
| } | ||
|
|
||
| /// Connects to a `Deref<Target = MetaIoHandler<Metadata + Default>`. | ||
| pub fn connect<TClient, THandler, TMetadata>( |
There was a problem hiding this comment.
hrm, this will connect without Middleware (Noop) because of MetaIoHandler<TMetadata>? It seems unintuitive to me but I understand that it wouldn't be backward compatible otherwise. Perhaps explain this with better documentation and remove it in v16 later?
There was a problem hiding this comment.
it will connect with the NoopMiddleware
There was a problem hiding this comment.
Yepp, that was what I meant with Middleware (Noop), but I was mostly referring to:
Connects to a
Deref<Target = MetaIoHandler<Metadata + Default>
It's not easy to understand that it's using the default associated type NoopMiddleware at least not to me but I guess it could be deduced by the other with_* methods.
niklasad1
left a comment
niklasad1
left a comment
There was a problem hiding this comment.
The repo is using cargo fmt please use it and the CI will pass.
seunlanlege
force-pushed
the
v15.0.1
branch
from
87d7550 to
ce1498a
Compare
There was a problem hiding this comment.
The PR is now mixing versions 15.1.0 and 15.0.1 which causes the build to fail.
IMHO, we should bump minor because it actually adds functionality in backward-compatible manner. If you don't agree please convince me :)
EDIT:
use ./_automate/bump_version.sh to bump versions
* v15.0.1 * v15.0.1 => v15.1.0 * v15.0.1 => v15.1.0 * cargo fmt * fix tests * adds *_with_middleware methods * remove Unpin bound, add documentation, cargo fmt * 15.0.1 * bump to 15.1.0 Co-authored-by: Seun Lanlege <seunlanlege@gmail.com> Co-authored-by: Niklas <niklasadolfsson1@gmail.com>
#589) (#600) * Update `local` client to support middleware (Kudos Seun) (#589) * v15.0.1 * v15.0.1 => v15.1.0 * v15.0.1 => v15.1.0 * cargo fmt * fix tests * adds *_with_middleware methods * remove Unpin bound, add documentation, cargo fmt * 15.0.1 * bump to 15.1.0 Co-authored-by: Seun Lanlege <seunlanlege@gmail.com> Co-authored-by: Niklas <niklasadolfsson1@gmail.com> * cargo fmt --all Co-authored-by: Seun Lanlege <seunlanlege@gmail.com> Co-authored-by: Niklas <niklasadolfsson1@gmail.com>
5 participants
Previously the
jsonrpc_core_client::transports::local::connectconsumed aDeref<Target=MetaIoHandler<TMetadata, NoopMiddleware>>,This PR corrects that behaviour allowing the local transport support other
Middlewareimplementations