-
Notifications
You must be signed in to change notification settings - Fork 656
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add an always
method on EventLoopFuture
#981
Merged
Merged
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
4d9771a
Add an `always` method on EventLoopFuture
c09cb6b
Merge branch 'master' into romain/add-always
55c8758
Merge branch 'master' into romain/add-always
Lukasa 839fe11
Pass the result as argument of the always closure
f122567
Merge branch 'master' into romain/add-always
Lukasa f22c1cb
Update EventLoopFuture.swift
weissi f1499a1
Merge branch 'master' into romain/add-always
Palleas abcfa3c
Merge branch 'master' into romain/add-always
weissi 8823fc4
Merge branch 'master' into romain/add-always
weissi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lukasa do you think we should give users the result here? For
whenComplete
we hand them the result so why not here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I just came across a case where having the above with the result (or
alwaysSuccess
/alwaysError
) would have been helpful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:Future<T>.always(_: () -> Void) -> Future<T>
Future<T>.always(_: () -> Void) -> Void
Future<T>.always(_: (Result<T, Error>) -> Void) -> Future<T>
Future<T>.always(_: (Result<T, Error>) -> Void) -> Void
Future<T>.always<U>(_: (Result<T, Error>) -> U) -> Future<U>
Future<T>.always<U>(_: (Result<T, Error>) -> Future<U>) -> Future<U>
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 whatwhenComplete
used to be in NIO 1, before we had aResult
type to use.(5) is arguably a function that would be called
mapBoth
, and (6) would beflatMapBoth
by analogy. (7) is akin to (4) but allows returning aFuture
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 likealwaysWithResult
. Otherwise we'll force a lot of people to writealways { _ in }
and that kinda sucks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lukasa Mostly agree, just that for
whenComplete
we decided to only offer the 'with result' case (but we didn't name itwhenCompleteWithResult
). Therefore, I'm not sure if foralways
we should doalways
andalwaysWithResult
... 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 withwhenComplete
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinion here, I'm happy to follow your preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weissi @Lukasa If there's a version providing the result then I'd think
alwaysWithSuccess
andalwaysWithError
would probably be requested too? Providing a value like this is pretty close to Java'speek
.If that's the case then maybe these are distinct from the
always
as it stands at the moment?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I kind of like the
peek
s. My only concern with leavingalways
as is would bealways
not giving you a value butwhenComplete
giving you the result. Just looks a bit inconsistent, that's all :). Especially that in NIO 1 we hadwhenComplete
giving you nothing and we switched towhenComplete
giving you the result for NIO2 :).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aResult
to the callback. The above would then be analogues of thewhen{Complete,Success,Failure,Result}
which return the future.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
when{Complete,Success,Failure,Result}
make sense to me. Filed #986There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Palleas just talked to @Lukasa about this. Let's add the
Result<...>
to the closure foralways
for consistency withwhenComplete
. The cleanup and providing the other missing methods can be done as #986 .