Skip to content

Conversation

@martinsik
Copy link
Contributor

This is the finally() operator based on its RxJS 5 implementation. I called in finallyCall() here because finally is a keyword but in RxPHP 2 I'll rename it to just finally().

@coveralls
Copy link

coveralls commented Jan 23, 2017

Coverage Status

Coverage increased (+0.05%) to 95.892% when pulling ba9c4a4 on martinsik:finally-operator into 2429fb7 on ReactiveX:master.

@felixfbecker
Copy link

In Promise implementations like reactphp or sabre this is typically called always

@davidwdan
Copy link
Member

@martinsik This is a do variant, so it might make sense to either call this finallyDo or doOnTerminate, which are names used by other Rx projects: http://reactivex.io/documentation/operators/do.html

If we want to stick to RxJS naming, finallyAction is an alias in RxJS 4.

@felixfbecker always doesn't make sense in this case, since we only want to call this when the observable sequence terminates, not each time it emits, which is what the name always implies.

@davidwdan
Copy link
Member

@martinsik it looks like there is a line break issue with the error demo with hhvm: https://travis-ci.org/ReactiveX/RxPHP/jobs/194568201#L341-L350

@felixfbecker
Copy link

doFinally sounds good

@davidwdan
Copy link
Member

Just to clarify, I'm only suggesting the name change for RxPHP v1x. I think that finallyDo makes the most sense. @mbonneau?

@felixfbecker
Copy link

Aren't all other do methods prefixed with do and not suffixed?

@mbonneau
Copy link
Member

I would go with finallyDo. I see no need to add a variant that doesn't already exist in any other rx project.

@martinsik
Copy link
Contributor Author

martinsik commented Jan 30, 2017

I think @felixfbecker has a good point. Isn't this going to be confusing when we'll have doOnEach, doOnNext, doOnError, doOnCompleted and then finallyDo?

@mbonneau
Copy link
Member

Just to move things along - I change my vote to doFinally

@martinsik
Copy link
Contributor Author

Ok, so I'll call it doFinally in RxPHP 1 and just finally in RxPHP 2.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 95.892% when pulling 4aa9450 on martinsik:finally-operator into 2429fb7 on ReactiveX:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 95.892% when pulling 4aa9450 on martinsik:finally-operator into 2429fb7 on ReactiveX:master.

@davidwdan davidwdan merged commit e2cfdb2 into ReactiveX:master Feb 1, 2017
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.

5 participants