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

Marshal mapping #3

Merged
merged 13 commits into from
Apr 12, 2017
Merged

Marshal mapping #3

merged 13 commits into from
Apr 12, 2017

Conversation

olejnjak
Copy link
Member

@olejnjak olejnjak commented Apr 6, 2017

Introduce mapResponseMarshal extension for SignalProducer.

olejnjak added 2 commits April 7, 2017 10:08
# Conflicts:
#	Example/ACKReactiveExtensions.xcodeproj/project.pbxproj
#	Example/Podfile.lock
#	Example/Pods/Manifest.lock
#	Example/Pods/Pods.xcodeproj/project.pbxproj
#	Example/Pods/Target Support Files/Pods-ACKReactiveExtensions_Example-ACKReactiveExtensions_Tests/Pods-ACKReactiveExtensions_Example-ACKReactiveExtensions_Tests.debug.xcconfig
#	Example/Pods/Target Support Files/Pods-ACKReactiveExtensions_Example-ACKReactiveExtensions_Tests/Pods-ACKReactiveExtensions_Example-ACKReactiveExtensions_Tests.release.xcconfig
#	README.md
@olejnjak
Copy link
Member Author

olejnjak commented Apr 7, 2017

Conflicts resolved

}
else if let marshaledArray = json as? [MarshaledObject] {
let dummyKey = "dummyKey"
return try [dummyKey: marshaledArray].value(for: dummyKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

try marshaledArray.map (Model.init)

}

extension MarshalError: MarshalErrorCreatable {
public static func createMarshalError(_ marshalError: MarshalError) -> MarshalError {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be _ marshalError: NSError?

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't see any reason why? As far as Marshal is throwing MarshalErrors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aaaaa I see...

Copy link
Member Author

Choose a reason for hiding this comment

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

Now when all errors thrown by the extension are really MarshalErrors this is correct.

guard let marshaledJSON = json as? MarshaledObject
else {
assertionFailure("json isn't any of the known MarshaledObject types (Dictionary or Array)")
throw NSError(domain: "", code: 0, userInfo: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get rid of this and throw some better error?

*
* - parameter key: If your objects are contained within dictionary pass the key here
*/
public func mapResponseMarshal<Model>(for key: KeyType? = nil) -> SignalProducer<Model, Error> where Model: Unmarshaling {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change it to forKey key: KeyType? = nil). It's not clear what for: means.

public func mapResponseMarshal<Model>(for key: KeyType) -> SignalProducer<Model, Error> where Model: ValueType {
return lift { $0.mapResponseMarshal(for: key) }
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be also nice to have some test to call the public API

Copy link
Member Author

@olejnjak olejnjak Apr 10, 2017

Choose a reason for hiding this comment

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

You mean something like this?

@janmisar janmisar merged commit 4abfee1 into master Apr 12, 2017
@olejnjak olejnjak deleted the marshal_mapping branch April 12, 2017 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants