Skip to content

Conversation

@swizard0
Copy link
Contributor

@swizard0 swizard0 commented Jun 6, 2018

It turned out to be small, but quite confusing refactoring.

  1. At first I changed Box-ed future to a generic type parameter P inside Heartbeat.
  2. Then moved new constructor out of Heartbeat impl in order to allow choosing P on the call site.
  3. Then moved heartbeat_pulse invocation from the constructor directly to the connect fn.
  4. And finally managed to craft connect return type like this:
impl Future<Item = (Self, Heartbeat<impl Future<Item = (), Error = io::Error>>), Error = io::Error> + Send + 'static

It probably could be rewritten in a more clear way but currently I don't see how.

@coveralls
Copy link

coveralls commented Jun 6, 2018

Coverage Status

Coverage increased (+0.2%) to 55.857% when pulling 360f445 on swizard0:pull_req_heartbeat_impl_trait into d67f86e on sozu-proxy:master.

Copy link
Collaborator

@Keruspe Keruspe left a comment

Choose a reason for hiding this comment

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

This is just me being nitpicky, but maybe rename "P" to "Pulse" and add the trait constraint ": Future + Send + 'static" for the struct and impl, mostly for "documentation" purpose.

Otherwise LGTM, thanks a lot!

/// ```
pub fn connect(stream: T, options: ConnectionOptions) -> impl Future<Item = (Self, Heartbeat), Error = io::Error> + Send + 'static {
pub fn connect(stream: T, options: ConnectionOptions) ->
impl Future<Item = (Self, Heartbeat<impl Future<Item = (), Error = io::Error>>), Error = io::Error> + Send + 'static
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add "+ Send + 'static" in the Heartbeat type param, just for consistency's sake?

@swizard0 swizard0 force-pushed the pull_req_heartbeat_impl_trait branch from 888bbe4 to 360f445 Compare June 7, 2018 10:10
@swizard0
Copy link
Contributor Author

swizard0 commented Jun 7, 2018

You're welcome.

I've made the fixes, currently except this:

and add the trait constraint ": Future + Send + 'static" for the struct and impl, mostly for "documentation" purpose

I'm not really sure that library users want to know this field type constraints because it is private and there is no way to get and use it for them.

Perhaps this unnecessary information only distracts the reader.

@Keruspe Keruspe merged commit ab21242 into amqp-rs:master Jun 8, 2018
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.

3 participants