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

New materialize() operator over BlockingObservable #1383

Merged
merged 11 commits into from
Aug 22, 2017

Conversation

sgleadow
Copy link
Contributor

This is based on a discussion in #1355 and on previous changes in #1354.

We introduce new extension to BlockingObservable called materialize() which returns a result type capturing the completed/failed status of the sequence, along with and elements and the error. The result type looks like this:

public enum SequenceMaterializeResult<T> {
    case completed(elements: [T])
    case failed(elements: [T], error: Error)
}

Internally, the rest of the BlockingObservable+Operators use SequenceMaterializeResult, and only throw at the last minute if an unexpected error occurs. This is mostly to maintain a backwards compatible interface for the existing operators, so it's a non-breaking change.

Copy link
Member

@freak4pc freak4pc left a comment

Choose a reason for hiding this comment

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

Looks really good, I just have one comment over naming that I'd like to discuss, and also I think it would be really nice to add a quick section on using this operator here: https://github.com/ReactiveX/RxSwift/blob/master/Documentation/UnitTests.md#integration-tests

@@ -10,14 +10,20 @@
import RxSwift
#endif

public enum SequenceMaterializeResult<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure on the naming here,
Would expect MaterializedSequenceResult<T> perhaps

Also wondering if it could be nice to go with elements and errors similar to RxSwiftExt's operators that extend materialize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rename totally make sense, I'll do that. In terms of errors do you mean in the enum case name or in the associated value parameter?

I'm not sure plural errors makes sense given it's only a single error that terminates the sequence? (I think, Rx newbie here).

  1. In case name:
public enum MaterializedSequenceResult<T> {
    case completed(elements: [T])
    case errors(elements: [T], error: Error)
}
  1. In associated value name
public enum MaterializedSequenceResult<T> {
    case completed(elements: [T])
    case failed(elements: [T], errors: [Error])
}

@sgleadow sgleadow changed the base branch from master to develop August 17, 2017 01:41
@sgleadow
Copy link
Contributor Author

Good point, examples added to UnitTests.md documentation

Copy link
Member

@freak4pc freak4pc left a comment

Choose a reason for hiding this comment

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

Cleaned up the documentation a bit, and after thinking this over night yesterday - .completed and .failed make sense to me. Even though there is a disparity between what materialize() returns in RxSwift vs RxBlocking, I think that should be fine mostly.

I think this PR is all good to go on my end. Let's wait for @kzaher to review this and merge :)

@freak4pc freak4pc requested a review from kzaher August 17, 2017 08:53
@freak4pc freak4pc changed the title Add materialize results to blocking observable New materialize() operator over BlockingObservable Aug 17, 2017
@freak4pc
Copy link
Member

@sgleadow Could you also real quickly add a CHANGELOG entry with something along the lines of "Added materialize() operator for RxBlocking's BlockingObservable`"

@sgleadow
Copy link
Contributor Author

Thanks for the help. I had no idea you could push to my fork!

I've added a line to the CHANGELOG referencing this PR.

@freak4pc
Copy link
Member

freak4pc commented Aug 17, 2017

@sgleadow Yeah you can do it only through GitHub's UI apparently but its sometimes helpful for README stuff :) Thanks for all the hard work! @sgleadow

Don't think there's anything blocking (pun intended) here, as soon as @kzaher finds the time

Copy link
Member

@kzaher kzaher left a comment

Choose a reason for hiding this comment

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

I've found some smaller items, but in general looks good.


switch result {
case .completed:
XCTFail("Expected result to be complete eith error, but result was successful.")
Copy link
Member

Choose a reason for hiding this comment

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

eith -> with?

@@ -10,14 +10,20 @@
import RxSwift
#endif

Copy link
Member

Choose a reason for hiding this comment

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

I think we should document this, what completed means, what failed means and what is this enum for.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Or your mean inline documentation block for that enum ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tweaked the UnitTests.md and added an example of the successful case, not just the error case.

Although if you mean inline docs like @freak4pc mentioned, I can add that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked through other parts of the RxSwift codebase and thought an inline comment describing the MaterializedSequenceResult was also appropriate, so I've added that also.

/// If the sequence terminates successfully, the result is represented
/// by `.completed` with the array of elements.
///
/// If the sequene terminates with error, the result is represented
Copy link
Member

Choose a reason for hiding this comment

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

Typo sequene


switch result {
case .completed:
XCTFail("Expected result to be complete eith error, but result was successful.")
Copy link
Member

Choose a reason for hiding this comment

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

Typo eith.

@kzaher kzaher merged commit 4469683 into ReactiveX:develop Aug 22, 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.

3 participants