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

[Feature] Add Task.requestJSONEncodable #1349

Merged
merged 6 commits into from Oct 8, 2017

Conversation

Projects
None yet
7 participants
@sunshinejr
Member

sunshinejr commented Oct 7, 2017

Resolves #1325. We had 2 PRs for this one, but both needed some enhancements and because we want to release 10.0.0-beta.1 in the near future, I decided to finish it myself while giving credits to both @LeLuckyVint and @afonsograca. Thank you guys so much because without you we wouldn't have this feature that fast in the next release!

Additionally, @afonsograca is already in @Moya/contributors, but @LeLuckyVint is not, so I invited you manually as well. Would love you guys to review this one!

@sunshinejr sunshinejr added this to the 10.0 milestone Oct 7, 2017

@sunshinejr sunshinejr requested review from devxoul, afonsograca and SD10 Oct 7, 2017

@MoyaBot

This comment has been minimized.

Show comment
Hide comment
@MoyaBot

MoyaBot Oct 7, 2017

SwiftLint found issues

Warnings

File Line Reason
MoyaProvider+Internal.swift 204 Function should have complexity 10 or less: currently complexity equals 14
MoyaProvider+Internal.swift 93 Function should have 5 parameters or less: it currently has 7
EndpointSpec.swift 268 Colons should be next to the identifier when specifying a type and next to the key in dictionary literals.
EndpointSpec.swift 269 Colons should be next to the identifier when specifying a type and next to the key in dictionary literals.

Generated by 🚫 Danger

MoyaBot commented Oct 7, 2017

SwiftLint found issues

Warnings

File Line Reason
MoyaProvider+Internal.swift 204 Function should have complexity 10 or less: currently complexity equals 14
MoyaProvider+Internal.swift 93 Function should have 5 parameters or less: it currently has 7
EndpointSpec.swift 268 Colons should be next to the identifier when specifying a type and next to the key in dictionary literals.
EndpointSpec.swift 269 Colons should be next to the identifier when specifying a type and next to the key in dictionary literals.

Generated by 🚫 Danger

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 7, 2017

Codecov Report

Merging #1349 into 10.0.0-dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           10.0.0-dev    #1349   +/-   ##
===========================================
  Coverage       88.41%   88.41%           
===========================================
  Files               5        5           
  Lines             164      164           
===========================================
  Hits              145      145           
  Misses             19       19

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 e4ddf0f...67463b8. Read the comment docs.

codecov-io commented Oct 7, 2017

Codecov Report

Merging #1349 into 10.0.0-dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           10.0.0-dev    #1349   +/-   ##
===========================================
  Coverage       88.41%   88.41%           
===========================================
  Files               5        5           
  Lines             164      164           
===========================================
  Hits              145      145           
  Misses             19       19

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 e4ddf0f...67463b8. Read the comment docs.

@@ -33,9 +33,11 @@ case .statusCode(let response):
print(response)
case .stringMapping(let response):
print(response)
case objectMapping(let error, let response):
case .objectMapping(let error, let response):

This comment has been minimized.

@devxoul

devxoul Oct 7, 2017

Member

Thanks!

@devxoul

devxoul Oct 7, 2017

Member

Thanks!

method throws three possible errors:
- `MoyaError.requestMapping(String)` when `URLRequest` could not be created for given path
- `MoyaError.parameterEncoding(Swift.Error)` when parameters couldn't be encoded
- `MoyaError.encodableMapping(Swift.Error)` when `Encodable` object couldn't be encoded into `Data`

This comment has been minimized.

@SD10

SD10 Oct 7, 2017

Member

Much cleaner 👍

@SD10

SD10 Oct 7, 2017

Member

Much cleaner 👍

@@ -77,6 +77,7 @@ public var task: Task {
- `.requestPlain` 没有任何东西发送
- `.requestData(_:)` 可以发送 `Data` (useful for `Encodable` types in Swift 4)
- `.requestJSONEncodable(_:)`
- `.requestParameters(parameters:encoding:)` 发送指定编码的参数

This comment has been minimized.

@SD10

SD10 Oct 7, 2017

Member

Bonus points for updating the Chinese docs 😱

@SD10

SD10 Oct 7, 2017

Member

Bonus points for updating the Chinese docs 😱

Show outdated Hide outdated Changelog.md Outdated
@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Oct 7, 2017

Member

I think this is great and a good way to handle both PRs. I do think this is better being listed as a breaking change because you are breaking the exhaustiveness of two enums 🤔

Member

SD10 commented Oct 7, 2017

I think this is great and a good way to handle both PRs. I do think this is better being listed as a breaking change because you are breaking the exhaustiveness of two enums 🤔

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 7, 2017

Member

True @SD10, we should probably list addition of new error cases as well.

Member

sunshinejr commented Oct 7, 2017

True @SD10, we should probably list addition of new error cases as well.

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Oct 7, 2017

Member

@sunshinejr Yeah, I didn't know if the policy was to keep a single entry per PR or to add multiple entries when dealing with an addition + a breaking change. I notice we didn't mention the new error case in #1335 as well.

Member

SD10 commented Oct 7, 2017

@sunshinejr Yeah, I didn't know if the policy was to keep a single entry per PR or to add multiple entries when dealing with an addition + a breaking change. I notice we didn't mention the new error case in #1335 as well.

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 7, 2017

Member

@pedrovereza @SD10 pushed my fixes, let me know how it looks now!

Member

sunshinejr commented Oct 7, 2017

@pedrovereza @SD10 pushed my fixes, let me know how it looks now!

@SD10

SD10 approved these changes Oct 7, 2017

Looks perfect 😃 Great job finding a solution that is friendly to the API

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 8, 2017

Member

Just a heads-up that I'll probably merge this later today as we plan to release 10.0.0-beta.1. So if anyone can/want to review this one, it will be greatly appreciated!

Member

sunshinejr commented Oct 8, 2017

Just a heads-up that I'll probably merge this later today as we plan to release 10.0.0-beta.1. So if anyone can/want to review this one, it will be greatly appreciated!

@sunshinejr sunshinejr merged commit d3b158c into 10.0.0-dev Oct 8, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@sunshinejr sunshinejr deleted the feature/parameter_encoding branch Oct 8, 2017

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