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 Recorded<Event<T>> array factory method #1531

Merged
merged 7 commits into from
Dec 22, 2017

Conversation

beeth0ven
Copy link
Contributor

@beeth0ven beeth0ven commented Dec 19, 2017

Hi there~~

This PR is an extension to #1510.

Introducing Recorded<Event<T>> array factory method:

extension Recorded {
    
    public static func events<T>(_ recordedEvents: Recorded<Event<T>>...) -> [Recorded<Event<T>>] 
        where Value == Event<T> { ... }
    
    public static func events<T>(_ recordedEvents: [Recorded<Event<T>>]) -> [Recorded<Event<T>>] 
        where Value == Event<T> { ...  }
}

This may help if you don't want to declare the type when create recorded events:

- let correctMessages: [Recorded<Event<Int>>] = [
-     .next(210, 2),
-     .next(220, 3),
-     .next(230, 4),
-     .next(240, 5),
-     .completed(250)
- ]

+ let correctMessages = Recorded.events(
+     .next(210, 2),
+     .next(220, 3),
+     .next(230, 4),
+     .next(240, 5),
+     .completed(250)
+ )

@RxPullRequestBot
Copy link

RxPullRequestBot commented Dec 19, 2017

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@beeth0ven
Copy link
Contributor Author

I'm not sure if we should use this method to create empty array or single element array?

// empty array
- let events: [Recorded<Event<Int>>] = []
+ let events = Recorded<Event<Int>>.events()

// single element array
- let messages = [
-    Recorded.error(250, testError, Int.self)
- ]
+ let messages = Recorded.events(
+    .error(250, testError, Int.self)
+ )

CHANGELOG.md Outdated
@@ -5,6 +5,7 @@ All notable changes to this project will be documented in this file.

## Master

* Add `Recorded<Event<T>>` arry factory method in **RxText**. #1531
Copy link
Member

Choose a reason for hiding this comment

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

Adds Recorded<Event<T>> array factory method in RxTest. #1531

extension Recorded {

/**
Factory method for an array of recorded events, this may help if you don't want to declare the type of array:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe formulate this sentence in some impersonal form:

Convenience method for recording a sequence of events. Its primary use case is improving readability in cases where type inference is unable to deduce the type of recorded events.

I'm hoping somebody will provide us some help with the docs, I'm not pleased with my formulation either.



/**
Factory method for an array of recorded events, this may help if you don't want to declare the type of array:
Copy link
Member

Choose a reason for hiding this comment

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

Same issue with docs.

]
```

- parameter recordedEvents: The recorded events which will return
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just say:

- parameter recordedEvents: Method return value.

]
```

- parameter recordedEvents: The recorded events which will return
Copy link
Member

Choose a reason for hiding this comment

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

Docs.

@beeth0ven
Copy link
Contributor Author

Thanks @kzaher

I've improved the documentation as you requested.

@kzaher kzaher merged commit 49a62eb into ReactiveX:develop Dec 22, 2017
@beeth0ven beeth0ven deleted the recorded-array-factory-method branch December 23, 2017 15:11
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