mapMany Operator #154

jdisho · 2018-05-06T00:50:26Z

For the proposed operator #93 🎉

freak4pc

Thanks for the PR! A few minor changes here :)

freak4pc · 2018-05-06T08:45:29Z

Playground/RxSwiftExtPlayground.playground/Pages/mapMany.xcplaygroundpage/Contents.swift

+            print(result)
+        })
+
+    stringsStream.mapMany { $0.lowercased() }


Can we add an example with a fake initializer (e.g. add a fake struct and initialize many� of it with mapMany) ?

freak4pc · 2018-05-06T08:49:07Z

Source/RxSwift/mapMany.swift

+     - returns: An observable collection whose elements are the result of invoking the transform function on each element of source.
+     */
+
+    public func mapMany<T>(_ transform: @escaping (E.Element) -> T) -> Observable<[T]> {


I think the transform block needs to throw like map itself: https://github.com/ReactiveX/RxSwift/blob/master/RxSwift/Observables/Map.swift#L20

What about the implementation example I put in #93 (comment) ?

It is a bit unclear why it should throw 🤔

Because the original map method accepts a throwing closure, so we should accept a throwing closure as well:
func map(_ transform: (Self.Element) throws -> T) rethrows -> [T]

freak4pc · 2018-05-06T08:49:52Z

Tests/RxSwift/MapManyTests.swift

+import RxTest
+
+class MapManyTests: XCTestCase {
+    fileprivate func runAndObserve<C: Collection>(_ sequence: Observable<C>) -> TestableObserver<C> {


Same here like the sample project - please add something that tests mass initialization of a data model, like a struct.

Thanks !

freak4pc · 2018-05-06T10:40:00Z

P.S @jdisho If you'll have time to do this in the next few days we can push it in the upcoming release :) Don't mind waiting since its a few minor fixes.

freak4pc · 2018-05-06T18:42:25Z

Source/RxSwift/mapMany.swift

@@ -16,7 +16,7 @@ extension ObservableType where E: Collection {
     - returns: An observable collection whose elements are the result of invoking the transform function on each element of source.
     */

-    public func mapMany<T>(_ transform: @escaping (E.Element) throws -> T) -> Observable<[T]> {
+    public func mapMany<T>(_ transform: @escaping (E.Element) throws -> T) rethrows -> Observable<[T]> {


There's no need for a rethrow IMO.
Look at the original signature from RxSwift map:

Don't mind reverting the commit, but a func can be a rethrowing func to show that it throws an error only if it’s function parameter throws an error. 🤔

Uhm if that would be the case than the map throws without rethrow doesn’t make sense ? I think in general we’d want it to behave much like a regular map so I think It’s best to keep the signature similar

freak4pc

Two minor changes and we're g2g

freak4pc · 2018-05-07T09:17:04Z

Tests/RxSwift/MapManyTests.swift

+}
+
+class MapManyTests: XCTestCase {
+    fileprivate func runAndObserve<C: Collection>(_ sequence: Observable<C>) -> TestableObserver<C> {


can you get rid of fileprivate here (and also on top) ?
It's irrelevant given the fact its part of a test suite, and the method is part of the class itself.

freak4pc · 2018-05-07T09:18:54Z

Playground/RxSwiftExtPlayground.playground/Pages/mapMany.xcplaygroundpage/Contents.swift

+import RxSwift
+import RxSwiftExt
+
+fileprivate struct SomeModel: CustomStringConvertible {


Same here, no reason for ACL at all given the fact its constrained to the playground page

toSortedArray operator

This reverts commit b9a699e.

freak4pc · 2018-05-07T20:31:34Z

@jdisho Did a bit of cleanup, let me know what you think :)
I'll do another pass of review tomorrow.

Also noticed some issue with toSortedArray playground page and missing CHANGELOG entry.

freak4pc · 2018-05-10T15:49:18Z

There seems to be a few missing operators in the playground page and README 🤔 not sure why. Need to find the time to look into it.

jdisho · 2018-07-08T12:30:03Z

Can this operator be merged anytime soon @freak4pc ? 😊

_{Sent with GitHawk}

freak4pc · 2018-07-08T14:26:20Z

OK so, there is some issue with documentation across other operators that were probably not reviewed properly before merge. We'll have to deal with it via a different PR but I think we can merge this regardless.

jdisho · 2018-07-08T14:28:09Z

I can update the documentation in a separate PR.

freak4pc · 2018-07-08T14:29:00Z

I'm gonna merge this as soon as CI passes.

And yeah that'd be great but to be honest i feel like we're going to have to add some automation to make sure documentation is synced across README -> Playgrounds -> Comments etc. Might be some work but we'll see later on :)

freak4pc · 2018-07-08T14:34:20Z

Something with your tests seem wrong, can you look into that?

jdisho · 2018-07-08T14:39:04Z

Strange, CI is successfully passed.😕

freak4pc · 2018-07-08T14:39:28Z

Yeah, the fact CI passes is a bug that you should ignore - but the build clearly fails

freak4pc · 2018-07-08T14:49:22Z

Ok, so CI is fixed - now time to fix your tests :) @jdisho

jdisho · 2018-07-08T15:31:44Z

I don't understand, the test passed in Xcode 😕

freak4pc · 2018-07-08T15:34:55Z

@jdisho Tried bumping CI Xcode to 9.4.0, lets see if that helps

jdisho · 2018-07-08T15:40:09Z

Apparently no

freak4pc · 2018-07-08T15:41:17Z

Here we go :) @jdisho

freak4pc · 2018-07-08T17:59:43Z

@jdisho Usually the author of the PR should not be the one to merge it, just FYI - but no biggie :)

jdisho added the proposed Operator label May 6, 2018

jdisho requested review from freak4pc and fpillet May 6, 2018 00:50

jdisho changed the title ~~mapMany~~ mapMany Operator May 6, 2018

freak4pc requested changes May 6, 2018

View reviewed changes

freak4pc mentioned this pull request May 6, 2018

Adding mapElements operator #93

Closed

jdisho force-pushed the feature/mapMany branch from cd34d7a to 5f057f3 Compare May 6, 2018 17:24

freak4pc reviewed May 6, 2018

View reviewed changes

freak4pc requested changes May 7, 2018

View reviewed changes

freak4pc and others added 11 commits May 7, 2018 22:58

Merge pull request #137 from feature/sorted

5158776

toSortedArray operator

create mapMany operator

c26b5c7

add playground example for mapMany

733465a

add tests

8a1959c

add a playgroud example and test case using a struct.

a134810

make mapMany a rethrowing function

7a79929

Revert "make mapMany a rethrowing function"

6b08ce9

This reverts commit b9a699e.

small minor changes

2c9b450

Minor cleanups

22ca3d8

Clean up tests

d48bad3

Fix issues with toSortedArray plagyround

c49ac48

freak4pc force-pushed the feature/mapMany branch from 386d188 to 7f117f0 Compare May 7, 2018 20:31

Add CHANGELOG entry

6127005

freak4pc force-pushed the feature/mapMany branch from 7f117f0 to 6127005 Compare May 8, 2018 08:24

freak4pc force-pushed the master branch from ac33491 to 2204a68 Compare May 28, 2018 15:53

Merge branch 'master' into feature/mapMany

7c5c9e9

freak4pc approved these changes Jul 8, 2018

View reviewed changes

freak4pc force-pushed the feature/mapMany branch 2 times, most recently from c672c14 to 7a9135b Compare July 8, 2018 14:55

Fix Circle CI to fix xcpretty exit code

0b067f9

freak4pc force-pushed the feature/mapMany branch from 7a9135b to 0b067f9 Compare July 8, 2018 15:34

Bump RxSwift to 4.2.0

093e765

jdisho merged commit 0ca281f into master Jul 8, 2018

freak4pc deleted the feature/mapMany branch July 8, 2018 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mapMany Operator #154

mapMany Operator #154

jdisho commented May 6, 2018

freak4pc left a comment

freak4pc May 6, 2018

freak4pc May 6, 2018

jdisho May 6, 2018

freak4pc May 6, 2018

freak4pc May 6, 2018

freak4pc commented May 6, 2018

freak4pc May 6, 2018 •

edited

jdisho May 6, 2018

freak4pc May 6, 2018

freak4pc left a comment

freak4pc May 7, 2018

freak4pc May 7, 2018

freak4pc commented May 7, 2018

freak4pc commented May 10, 2018

jdisho commented Jul 8, 2018

freak4pc commented Jul 8, 2018

jdisho commented Jul 8, 2018

freak4pc commented Jul 8, 2018

freak4pc commented Jul 8, 2018

jdisho commented Jul 8, 2018

freak4pc commented Jul 8, 2018

freak4pc commented Jul 8, 2018

jdisho commented Jul 8, 2018

freak4pc commented Jul 8, 2018

jdisho commented Jul 8, 2018

freak4pc commented Jul 8, 2018

freak4pc commented Jul 8, 2018

mapMany Operator #154

mapMany Operator #154

Conversation

jdisho commented May 6, 2018

freak4pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freak4pc commented May 6, 2018

freak4pc May 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freak4pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freak4pc commented May 7, 2018

freak4pc commented May 10, 2018

jdisho commented Jul 8, 2018

freak4pc commented Jul 8, 2018

jdisho commented Jul 8, 2018

freak4pc commented Jul 8, 2018

freak4pc commented Jul 8, 2018

jdisho commented Jul 8, 2018

freak4pc commented Jul 8, 2018

freak4pc commented Jul 8, 2018

jdisho commented Jul 8, 2018

freak4pc commented Jul 8, 2018

jdisho commented Jul 8, 2018

freak4pc commented Jul 8, 2018

freak4pc commented Jul 8, 2018

freak4pc May 6, 2018 •

edited