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

Add an `always` method on EventLoopFuture #981

Merged
merged 9 commits into from May 15, 2019

Conversation

Projects
None yet
5 participants
@Palleas
Copy link
Contributor

commented Apr 27, 2019

Motivation:

#665

Modifications:

  • Add always in EventLoopFuture.swift
  • Add tests

Result:

NIO Users will be able to use .always to, well, always run an action
whether the future succeed or not.

@swift-nio-bot

This comment has been minimized.

Copy link

commented Apr 27, 2019

Can one of the admins verify this patch?

4 similar comments
@swift-nio-bot

This comment has been minimized.

Copy link

commented Apr 27, 2019

Can one of the admins verify this patch?

@swift-nio-bot

This comment has been minimized.

Copy link

commented Apr 27, 2019

Can one of the admins verify this patch?

@swift-nio-bot

This comment has been minimized.

Copy link

commented Apr 27, 2019

Can one of the admins verify this patch?

@swift-nio-bot

This comment has been minimized.

Copy link

commented Apr 27, 2019

Can one of the admins verify this patch?

@Lukasa
Copy link
Contributor

left a comment

One minor nit, otherwise looks great.



extension EventLoopFuture {

This comment has been minimized.

Copy link
@Lukasa

Lukasa Apr 29, 2019

Contributor

Minor nit: can we remove this whitespace?

This comment has been minimized.

Copy link
@Palleas

Palleas Apr 29, 2019

Author Contributor

@Lukasa Done!

@Lukasa Lukasa added this to the 2.1.0 milestone Apr 29, 2019

@Palleas Palleas force-pushed the Palleas:romain/add-always branch 2 times, most recently from 7778e37 to 0195ebc Apr 29, 2019

@Lukasa

Lukasa approved these changes Apr 29, 2019

Copy link
Contributor

left a comment

Looks good to me, I'll get @weissi to review as well.

@Lukasa Lukasa requested a review from weissi Apr 29, 2019

/// - parameters:
/// - callback: the callback that is called when the `EventLoopFuture` is fulfilled.
/// - returns: the current `EventLoopFuture`
public func always(_ callback: @escaping () -> Void) -> EventLoopFuture<Value> {

This comment has been minimized.

Copy link
@weissi

weissi Apr 30, 2019

Member

@Lukasa do you think we should give users the result here? For whenComplete we hand them the result so why not here?

This comment has been minimized.

Copy link
@glbrntt

glbrntt Apr 30, 2019

Contributor

FWIW I just came across a case where having the above with the result (or alwaysSuccess/alwaysError) would have been helpful.

This comment has been minimized.

Copy link
@Lukasa

Lukasa Apr 30, 2019

Contributor

I don't think I mind particularly. Arguably there are a family of functions here (giving them the name always for now though the compiler wouldn't like it much) created by twiddling both the return value of the function and the return value of the closure it accepts:

  1. Future<T>.always(_: () -> Void) -> Future<T>
  2. Future<T>.always(_: () -> Void) -> Void
  3. Future<T>.always(_: (Result<T, Error>) -> Void) -> Future<T>
  4. Future<T>.always(_: (Result<T, Error>) -> Void) -> Void
  5. Future<T>.always<U>(_: (Result<T, Error>) -> U) -> Future<U>
  6. Future<T>.always<U>(_: (Result<T, Error>) -> Future<U>) -> Future<U>
  7. Future<T>.always(_: (Result<T, Error>) -> Future<Void>) -> Future<T>

This proposal covers (1). (4) is currently spelled whenComplete. (3) is what you're proposing instead, and can do everything (1) can do at the cost of being a bit noisier in the code when you want (1). (2) is what whenComplete used to be in NIO 1, before we had a Result type to use.

(5) is arguably a function that would be called mapBoth, and (6) would be flatMapBoth by analogy. (7) is akin to (4) but allows returning a Future while persisting the result of the computation.

I think there could be a case made for providing all of these functions: the biggest challenge would be providing descriptive names, as we cannot rely on the compiler to provide overload resolution. (7) is the least obviously useful.

I think we should stick with always being the name for this function. We should also provide (3) and call it something like alwaysWithResult. Otherwise we'll force a lot of people to write always { _ in } and that kinda sucks.

This comment has been minimized.

Copy link
@weissi

weissi Apr 30, 2019

Member

@Lukasa Mostly agree, just that for whenComplete we decided to only offer the 'with result' case (but we didn't name it whenCompleteWithResult). Therefore, I'm not sure if for always we should do always and alwaysWithResult... But I agree that the _ in kinda sucks and obviously that the compiler won't like overloading at all. I'd probably still go for consistency with whenComplete.

This comment has been minimized.

Copy link
@Lukasa

Lukasa Apr 30, 2019

Contributor

No strong opinion here, I'm happy to follow your preference.

This comment has been minimized.

Copy link
@glbrntt

glbrntt Apr 30, 2019

Contributor

@weissi @Lukasa If there's a version providing the result then I'd think alwaysWithSuccess and alwaysWithError would probably be requested too? Providing a value like this is pretty close to Java's peek.

If that's the case then maybe these are distinct from the always as it stands at the moment?

Future<T>.always(_: () -> Void) -> Future<T>
Future<T>.peek(_: (T) -> Void) -> Future<T>
Future<T>.peekError(_: (Error) -> Void) -> Future<T>
Future<T>.peekResult(_: (Result<T, Error>) -> Void) -> Future<T>

This comment has been minimized.

Copy link
@weissi

weissi Apr 30, 2019

Member

Heh, I kind of like the peeks. My only concern with leaving always as is would be always not giving you a value but whenComplete giving you the result. Just looks a bit inconsistent, that's all :). Especially that in NIO 1 we had whenComplete giving you nothing and we switched to whenComplete giving you the result for NIO2 :).

This comment has been minimized.

Copy link
@glbrntt

glbrntt Apr 30, 2019

Contributor

True, it's probably best to be consistent with whenComplete even though I prefer the NIO 1 version of it! I think I ignore the result more often than not...

I would vote to change it back in some future version of NIO and introduce whenResult to be closer in name to the other methods which provide a Result to the callback. The above would then be analogues of the when{Complete,Success,Failure,Result} which return the future.

This comment has been minimized.

Copy link
@weissi

weissi Apr 30, 2019

Member

Sounds good. when{Complete,Success,Failure,Result} make sense to me. Filed #986

This comment has been minimized.

Copy link
@weissi

weissi May 8, 2019

Member

@Palleas just talked to @Lukasa about this. Let's add the Result<...> to the closure for always for consistency with whenComplete. The cleanup and providing the other missing methods can be done as #986 .

@Palleas Palleas force-pushed the Palleas:romain/add-always branch 2 times, most recently from a87379a to aaadfba Apr 30, 2019

@Palleas

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@Lukasa fixed the failing build, sorry about that. I'm slowly engraving in my brain the reflex of running the sanity check locally.

Add an `always` method on EventLoopFuture
Motivation:

#665

Modifications:

* Add `always` in EventLoopFuture.swift
* Add tests

Result:

NIO Users will be able to use `.always` to, well, always run an action
wether the future succeed or not.

@Palleas Palleas force-pushed the Palleas:romain/add-always branch from aaadfba to 4d9771a Apr 30, 2019

Palleas and others added some commits May 2, 2019

@weissi
Copy link
Member

left a comment

Thank you! This looks mostly good but we should add the Result for consistency

Pass the result as argument of the always closure
Motivation:

Can be useful to developers and is consistent with the rest of the
codebase

Modifications:

Add `Result<Value, Error>` to the callback

Result:

Result is available
@Palleas

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@weissi your wish is my command 🙇

@weissi

weissi approved these changes May 9, 2019

Copy link
Member

left a comment

Thank you! Looks good to me.

@Lukasa Lukasa modified the milestones: 2.1.0, 2.2.0 May 9, 2019

Palleas and others added some commits May 9, 2019

@weissi weissi merged commit c59c193 into apple:master May 15, 2019

2 checks passed

pull request validation (5.0) Build finished.
Details
pull request validation (5.1) Build finished.
Details

@Palleas Palleas deleted the Palleas:romain/add-always branch May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.