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 support for mapping response to Decodable object #1335

Merged
merged 8 commits into from Oct 6, 2017

Conversation

Projects
None yet
7 participants
@devxoul
Member

devxoul commented Oct 1, 2017

  • Resolves #1118

  • I recommend you to see commit logs first before reading code.

  • Adds MoyaError.objectMapping(Error, Response) which contains a DecodingError to make it easy to inspect a decoding error.

  • Interfaces:

    // Response -> D
    func map<D: Decodable>(_ type: D.Type, atKeyPath keyPath: String? = nil, using decoder: JSONDecoder) throws -> D
    
    // Obsevable<Response> -> Observable<D>
    func map<D: Decodable>(_ type: D.Type, atKeyPath keyPath: String? = nil, using decoder: JSONDecoder) throws -> Observable<D>
    
    // Single<Response> -> Single<D>
    func map<D: Decodable>(_ type: D.Type, atKeyPath keyPath: String? = nil, using decoder: JSONDecoder) throws -> Single<D>
    
    // SignalProducer<Response, MoyaError> -> SignalProducer<D, MoyaError>
    func map<D: Decodable>(_ type: D.Type, atKeyPath keyPath: String? = nil, using decoder: JSONDecoder) throws -> SignalProducer<D, MoyaError>
@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 1, 2017

Member

Thank you so much @devxoul for this one! 🎉 I didn't go through all of this yet, but few questions at a first glance:

  • How do you feel about mapping arrays of Decodables?
  • How do you feel about renaming mapObject(_:atKeyPath:using) to map(to:keyPath:decoder)?
  • How do you feel about having default decoder argument?
Member

sunshinejr commented Oct 1, 2017

Thank you so much @devxoul for this one! 🎉 I didn't go through all of this yet, but few questions at a first glance:

  • How do you feel about mapping arrays of Decodables?
  • How do you feel about renaming mapObject(_:atKeyPath:using) to map(to:keyPath:decoder)?
  • How do you feel about having default decoder argument?
@devxoul

This comment has been minimized.

Show comment
Hide comment
@devxoul

devxoul Oct 1, 2017

Member

@sunshinejr Thanks for quick review!

  1. [Decodable] is also a decodable so we do not have to write an extra function to deal with an array.
  2. I brought interfaces from RxCodable, but I can change it if you think it would be better. BTW, you mean decoder, not an encoder right? 😃
  3. That makes sense.

I updated existing commits :)

Member

devxoul commented Oct 1, 2017

@sunshinejr Thanks for quick review!

  1. [Decodable] is also a decodable so we do not have to write an extra function to deal with an array.
  2. I brought interfaces from RxCodable, but I can change it if you think it would be better. BTW, you mean decoder, not an encoder right? 😃
  3. That makes sense.

I updated existing commits :)

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 1, 2017

Member

@devxoul Definitely, I was talking about decoder but my mind got messed up, sorry!

I didn't know that [Decodable] is also Decodable - that's awesome! So now the question is for the interface, I don't mind both(or mixture) of them, so maybe let's wait for more inputs :)

Member

sunshinejr commented Oct 1, 2017

@devxoul Definitely, I was talking about decoder but my mind got messed up, sorry!

I didn't know that [Decodable] is also Decodable - that's awesome! So now the question is for the interface, I don't mind both(or mixture) of them, so maybe let's wait for more inputs :)

@MoyaBot

This comment has been minimized.

Show comment
Hide comment
@MoyaBot

MoyaBot Oct 1, 2017

SwiftLint found issues

Warnings

File Line Reason
Observable+MoyaSpec.swift 6 Type body should span 200 lines or less excluding comments and whitespace: currently spans 229 lines
SignalProducer+MoyaSpec.swift 10 Type body should span 200 lines or less excluding comments and whitespace: currently spans 213 lines
Single+MoyaSpec.swift 6 Type body should span 200 lines or less excluding comments and whitespace: currently spans 217 lines
TestHelpers.swift 55 Operators should be surrounded by a single whitespace when defining them.

Generated by 🚫 Danger

MoyaBot commented Oct 1, 2017

SwiftLint found issues

Warnings

File Line Reason
Observable+MoyaSpec.swift 6 Type body should span 200 lines or less excluding comments and whitespace: currently spans 229 lines
SignalProducer+MoyaSpec.swift 10 Type body should span 200 lines or less excluding comments and whitespace: currently spans 213 lines
Single+MoyaSpec.swift 6 Type body should span 200 lines or less excluding comments and whitespace: currently spans 217 lines
TestHelpers.swift 55 Operators should be surrounded by a single whitespace when defining them.

Generated by 🚫 Danger

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 1, 2017

Codecov Report

Merging #1335 into 10.0.0-dev will increase coverage by 6.47%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##           10.0.0-dev    #1335      +/-   ##
==============================================
+ Coverage       81.93%   88.41%   +6.47%     
==============================================
  Files               5        5              
  Lines             155      164       +9     
==============================================
+ Hits              127      145      +18     
+ Misses             28       19       -9
Impacted Files Coverage Δ
Sources/ReactiveMoya/SignalProducer+Response.swift 90.9% <100%> (+10.9%) ⬆️
Sources/RxMoya/Observable+Response.swift 70.58% <100%> (+12.52%) ⬆️
Sources/RxMoya/Single+Response.swift 100% <100%> (+14.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fa5482...9281b58. Read the comment docs.

codecov-io commented Oct 1, 2017

Codecov Report

Merging #1335 into 10.0.0-dev will increase coverage by 6.47%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##           10.0.0-dev    #1335      +/-   ##
==============================================
+ Coverage       81.93%   88.41%   +6.47%     
==============================================
  Files               5        5              
  Lines             155      164       +9     
==============================================
+ Hits              127      145      +18     
+ Misses             28       19       -9
Impacted Files Coverage Δ
Sources/ReactiveMoya/SignalProducer+Response.swift 90.9% <100%> (+10.9%) ⬆️
Sources/RxMoya/Observable+Response.swift 70.58% <100%> (+12.52%) ⬆️
Sources/RxMoya/Single+Response.swift 100% <100%> (+14.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fa5482...9281b58. Read the comment docs.

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Oct 1, 2017

Member

@devxoul 💯 Thank you for implementing this, you're a life-saver 👍 Thank you for writing tests too!
I can help with updating any docs that revolve around these changes.

I do like @sunshinejr's suggestion of naming the method map(to:) over mapObject. Object seems redundant and intuitive. I also like shorter names. However, I do like keeping the external parameter names the same as the original method:

func map<D: Decodable>(to: D.Type, atKeyPath keyPath: String? = nil, using decoder: JSONDecoder) throws -> D

The reason being is because this method reads as fluent English using the atKeyPath and using parameter names. 🤔

🚲 Shedding


👍 on using a default decoder argument as well

Member

SD10 commented Oct 1, 2017

@devxoul 💯 Thank you for implementing this, you're a life-saver 👍 Thank you for writing tests too!
I can help with updating any docs that revolve around these changes.

I do like @sunshinejr's suggestion of naming the method map(to:) over mapObject. Object seems redundant and intuitive. I also like shorter names. However, I do like keeping the external parameter names the same as the original method:

func map<D: Decodable>(to: D.Type, atKeyPath keyPath: String? = nil, using decoder: JSONDecoder) throws -> D

The reason being is because this method reads as fluent English using the atKeyPath and using parameter names. 🤔

🚲 Shedding


👍 on using a default decoder argument as well

@sunshinejr sunshinejr referenced this pull request Oct 1, 2017

Closed

Release 10.0 #1253

9 of 11 tasks complete
@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 1, 2017

Member

I'm up for mixed strategy as well (for interface naming), which is:

func map<D: Decodable>(to: D.Type, atKeyPath keyPath: String? = nil, using decoder: JSONDecoder = JSONDecoder()) throws -> D

So that will be the last thing before documentation (Changelog + documenting the option itself) 👍

Oh, and can you also fix linting issues (these that you can without bigger refactor), please?

Member

sunshinejr commented Oct 1, 2017

I'm up for mixed strategy as well (for interface naming), which is:

func map<D: Decodable>(to: D.Type, atKeyPath keyPath: String? = nil, using decoder: JSONDecoder = JSONDecoder()) throws -> D

So that will be the last thing before documentation (Changelog + documenting the option itself) 👍

Oh, and can you also fix linting issues (these that you can without bigger refactor), please?

@devxoul

This comment has been minimized.

Show comment
Hide comment
@devxoul

devxoul Oct 2, 2017

Member

I like that interface. It looks much better 👍 By the way, how about removing the parameter name to? Because, we're using such like mapData() or mapString() over mapToData() and mapToString(). English is not my first language but map something seems to be more fluent to me than map to something.

- foo.map(to: Issue.self, atKeyPath: "data", using: myDecoder)
+ foo.map(Issue.self, atKeyPath: "data", using: myDecoder)

@sunshinejr, Hmm, there are several issues on linting issues:

  1. Quick spec is a single class and a single method, so it can easily exceed 200 lines. How about removing this check from the test code?
  2. The 55th line of TestHelpers.swift is not included in this pull request. I think we should fix this in another pull request to keep this pr tidy. How do you think?
Member

devxoul commented Oct 2, 2017

I like that interface. It looks much better 👍 By the way, how about removing the parameter name to? Because, we're using such like mapData() or mapString() over mapToData() and mapToString(). English is not my first language but map something seems to be more fluent to me than map to something.

- foo.map(to: Issue.self, atKeyPath: "data", using: myDecoder)
+ foo.map(Issue.self, atKeyPath: "data", using: myDecoder)

@sunshinejr, Hmm, there are several issues on linting issues:

  1. Quick spec is a single class and a single method, so it can easily exceed 200 lines. How about removing this check from the test code?
  2. The 55th line of TestHelpers.swift is not included in this pull request. I think we should fix this in another pull request to keep this pr tidy. How do you think?
@phimage

This comment has been minimized.

Show comment
Hide comment
@phimage

phimage Oct 2, 2017

Contributor

The Decodable type is not mandatory as parameter of the function
You could specify it on object result type

let response = Response(statusCode: 200, data: data)

let receivedObject: Issue = try response.mapObject(atKeyPath: "issue", decoder: decoder)

Maybe it's easier for reactivate programming but for standard behaviour I thinks removing it will be the best solution event if the apple function is decode<T>(_ type: T.Type, from data: Data)

With reactive programming the code will be something like this

let single = response.asSingle()
var receivedObject: Issue?
_ = single.mapObject(atKeyPath: "issue", decoder: decoder).subscribe(onSuccess: { (object: Issue) in
      receivedObject = object
})

for event , .subscribe { (event: (SingleEvent<Issue>)) in

Contributor

phimage commented Oct 2, 2017

The Decodable type is not mandatory as parameter of the function
You could specify it on object result type

let response = Response(statusCode: 200, data: data)

let receivedObject: Issue = try response.mapObject(atKeyPath: "issue", decoder: decoder)

Maybe it's easier for reactivate programming but for standard behaviour I thinks removing it will be the best solution event if the apple function is decode<T>(_ type: T.Type, from data: Data)

With reactive programming the code will be something like this

let single = response.asSingle()
var receivedObject: Issue?
_ = single.mapObject(atKeyPath: "issue", decoder: decoder).subscribe(onSuccess: { (object: Issue) in
      receivedObject = object
})

for event , .subscribe { (event: (SingleEvent<Issue>)) in

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Oct 2, 2017

Member

Wow, very interesting @phimage. Maybe we can keep the type constraint on the reactive methods and drop it on the normal Moya.Response method? I also wonder how obvious it will be that you need to specify the type explicitly 🤔

Member

SD10 commented Oct 2, 2017

Wow, very interesting @phimage. Maybe we can keep the type constraint on the reactive methods and drop it on the normal Moya.Response method? I also wonder how obvious it will be that you need to specify the type explicitly 🤔

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 2, 2017

Member

👍 for removing to parameter name

About skipping type parameter - I feel like this would make more issues about compiler problems as you need to explicitly write that type to let compiler infer it. Making it explicit with a parameter removes this problem as people would have to specify it always as a parameter.

Member

sunshinejr commented Oct 2, 2017

👍 for removing to parameter name

About skipping type parameter - I feel like this would make more issues about compiler problems as you need to explicitly write that type to let compiler infer it. Making it explicit with a parameter removes this problem as people would have to specify it always as a parameter.

@devxoul

This comment has been minimized.

Show comment
Hide comment
@devxoul

devxoul Oct 2, 2017

Member

If we remove the type parameter, method chaining will not work properly because the compiler cannot infer return types without an explicit type annotation.

Member

devxoul commented Oct 2, 2017

If we remove the type parameter, method chaining will not work properly because the compiler cannot infer return types without an explicit type annotation.

@@ -266,5 +266,87 @@ class ObservableMoyaSpec: QuickSpec {
expect(receivedError).to(beOfSameErrorType(expectedError))
}
}
describe("object mapping") {

This comment has been minimized.

@sunshinejr

sunshinejr Oct 2, 2017

Member

Can we add a test that maps an array of objects?

@sunshinejr

sunshinejr Oct 2, 2017

Member

Can we add a test that maps an array of objects?

This comment has been minimized.

@devxoul

devxoul Oct 3, 2017

Member

Sure! Added.

@devxoul

devxoul Oct 3, 2017

Member

Sure! Added.

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 2, 2017

Member

Also, I'm sorry again @devxoul but I had to rebase 10.0.0-dev again as we had project changes that were needed on master. You will need to rebase again 😢

Member

sunshinejr commented Oct 2, 2017

Also, I'm sorry again @devxoul but I had to rebase 10.0.0-dev again as we had project changes that were needed on master. You will need to rebase again 😢

@devxoul

This comment has been minimized.

Show comment
Hide comment
@devxoul

devxoul Oct 3, 2017

Member

@sunshinejr, could you help me with this? Several files are missing from the project after rebasing.

screen shot 2017-10-03 at 2 49 31 pm

Member

devxoul commented Oct 3, 2017

@sunshinejr, could you help me with this? Several files are missing from the project after rebasing.

screen shot 2017-10-03 at 2 49 31 pm

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Oct 3, 2017

Member

@devxoul It's most likely because of #1292. Looks like the Xcodeproj was corrupted in the rebase

Member

SD10 commented Oct 3, 2017

@devxoul It's most likely because of #1292. Looks like the Xcodeproj was corrupted in the rebase

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 3, 2017

Member

Yep, working on it!

Member

sunshinejr commented Oct 3, 2017

Yep, working on it!

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 3, 2017

Member

@devxoul I think it is fixed, please try now. It turned out that we had both on 10.0.0-dev and master changes to pbxproj and automerge kinda broke it 😞

Member

sunshinejr commented Oct 3, 2017

@devxoul I think it is fixed, please try now. It turned out that we had both on 10.0.0-dev and master changes to pbxproj and automerge kinda broke it 😞

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Oct 3, 2017

Member

I believe I fixed it on a local copy if anyone needs it. It's essentially copying the project.pbxproj off of master and deleting the two references to RxMoya+Availability & ReactiveMoya+Availability.

EDIT: I didn't see your post 😅 Resolving the conflict via GitHub should fix it~. Same changes I made.

Member

SD10 commented Oct 3, 2017

I believe I fixed it on a local copy if anyone needs it. It's essentially copying the project.pbxproj off of master and deleting the two references to RxMoya+Availability & ReactiveMoya+Availability.

EDIT: I didn't see your post 😅 Resolving the conflict via GitHub should fix it~. Same changes I made.

@devxoul

This comment has been minimized.

Show comment
Hide comment
@devxoul

devxoul Oct 3, 2017

Member

@sunshinejr @SD10 thanks for quick support! I rebased 10.0.0-dev branch again and added some test code for json arrays.

Member

devxoul commented Oct 3, 2017

@sunshinejr @SD10 thanks for quick support! I rebased 10.0.0-dev branch again and added some test code for json arrays.

Show outdated Hide outdated Sources/Moya/Response.swift Outdated
Show outdated Hide outdated Tests/Observable+MoyaSpec.swift Outdated
Show outdated Hide outdated Tests/Observable+MoyaSpec.swift Outdated
@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 3, 2017

Member

@devxoul last comments from me!

Member

sunshinejr commented Oct 3, 2017

@devxoul last comments from me!

Show outdated Hide outdated Tests/TestHelpers.swift Outdated
@AndrewSB

@devxoul this is 🔥-y. Thank you

@devxoul

This comment has been minimized.

Show comment
Hide comment
@devxoul

devxoul Oct 4, 2017

Member

@sunshinejr @AndrewSB Thanks for your reviews! I just updated my commits :)

Member

devxoul commented Oct 4, 2017

@sunshinejr @AndrewSB Thanks for your reviews! I just updated my commits :)

@sunshinejr

Looks good to me! Thanks for this one! 🎉 I think that someone else should approve this one too, so I'm not merging this yet 😉

@SD10

SD10 approved these changes Oct 4, 2017

Thank you so much for doing this @devxoul 🥇 🎉

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Oct 4, 2017

Member

This does require a change to the rarely viewedErrorTypes.md but it can be done when we provide some sort of example documentation for the response decoding.

Member

SD10 commented Oct 4, 2017

This does require a change to the rarely viewedErrorTypes.md but it can be done when we provide some sort of example documentation for the response decoding.

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 4, 2017

Member

Oh, Jeon, could you also add entry to Changelog, please?

Member

sunshinejr commented Oct 4, 2017

Oh, Jeon, could you also add entry to Changelog, please?

@devxoul

This comment has been minimized.

Show comment
Hide comment
@devxoul

devxoul Oct 4, 2017

Member

Updated ErrorTypes.md and Changelog.md :)

Member

devxoul commented Oct 4, 2017

Updated ErrorTypes.md and Changelog.md :)

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Oct 4, 2017

Member

@devxoul 👍 on updating ErrorTypes.md. About the CHANGELOG entry, I think that adding MoyaError.objectMapping(Error, Response) is a breaking change because the MoyaError enum will no longer be considered exhaustive in some cases. Could you add an extra entry for this change?

Member

SD10 commented Oct 4, 2017

@devxoul 👍 on updating ErrorTypes.md. About the CHANGELOG entry, I think that adding MoyaError.objectMapping(Error, Response) is a breaking change because the MoyaError enum will no longer be considered exhaustive in some cases. Could you add an extra entry for this change?

Show outdated Hide outdated Changelog.md Outdated
@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 6, 2017

Member

Thank you again, @devxoul and all contributors in this one! 🎉

Member

sunshinejr commented Oct 6, 2017

Thank you again, @devxoul and all contributors in this one! 🎉

@sunshinejr sunshinejr merged commit c563689 into Moya:10.0.0-dev Oct 6, 2017

1 check was pending

ci/circleci CircleCI is running your tests
Details

afonsograca added a commit to afonsograca/Moya that referenced this pull request Oct 7, 2017

Merge pull request Moya#1335 from devxoul/map-decodable
Add support for mapping response to Decodable object

afonsograca added a commit to afonsograca/Moya that referenced this pull request Oct 7, 2017

# This is a combination of 3 commits.
# This is the 1st commit message:

Merge pull request Moya#1335 from devxoul/map-decodable

Add support for mapping response to Decodable object
# The commit message Moya#2 will be skipped:

# feat: add support for JSON Encodable requests.

# The commit message Moya#3 will be skipped:

# chore: add changelog entry and added documentation regarding the new request

@devxoul devxoul deleted the devxoul:map-decodable branch Oct 7, 2017

@devxoul

This comment has been minimized.

Show comment
Hide comment
@devxoul

devxoul Oct 7, 2017

Member

Thanks for giving me the opportunity to contribute 🎉

Member

devxoul commented Oct 7, 2017

Thanks for giving me the opportunity to contribute 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment