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 response decoding using Decodable #113

Merged
merged 2 commits into from
Dec 7, 2018

Conversation

guilhermearaujo
Copy link
Contributor

As requested on #93

@guilhermearaujo
Copy link
Contributor Author

guilhermearaujo commented Dec 5, 2018

There seems to be a flaky test that fails eventually when I run them locally:

Test Case '-[Malibu_macOS_Tests.JsonEncoderSpec JsonEncoder___encode_parameters__encodes_a_dictionary_of_parameters_to_NSData_object]' started.
./MalibuTests/Specs/Encoding/JsonEncoderSpec.swift:22: error: -[Malibu_macOS_Tests.JsonEncoderSpec JsonEncoder___encode_parameters__encodes_a_dictionary_of_parameters_to_NSData_object] : expected to equal <Data<hash=115512365,length=37>>, got <Data<hash=125446637,length=37>>

Most of the attempts succeed, though. An identical Travis job on my fork also passed.

Copy link
Owner

@vadymmarkov vadymmarkov left a comment

Choose a reason for hiding this comment

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

@guilhermearaujo Thanks for the PR! Left one comment, other than that looks great.


public extension Promise where T: Response {
@available(swift 4.1)
func decode<U: Decodable>(
Copy link
Owner

Choose a reason for hiding this comment

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

How about:

fun decode<U: Decodable>(_ type: U.Type, using decoder: JSONDecoder = JSONDecoder()) {
    // ...
}

Then you can pass your custom decoder configured with data and date decoding strategy if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea, I'll change it.

@vadymmarkov
Copy link
Owner

That flaky test can be fixed in a separate PR I think.

@codecov-io
Copy link

Codecov Report

Merging #113 into master will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
+ Coverage   92.58%   92.73%   +0.15%     
==========================================
  Files          62       64       +2     
  Lines        3046     3110      +64     
==========================================
+ Hits         2820     2884      +64     
  Misses        226      226
Impacted Files Coverage Δ
Sources/NetworkError.swift 91.17% <100%> (+0.55%) ⬆️
...ibuTests/Specs/Response/ResponseDecodingSpec.swift 100% <100%> (ø)
Sources/Response/ResponseDecoding.swift 100% <100%> (ø)

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 cdb400e...fdea449. Read the comment docs.

Copy link
Owner

@vadymmarkov vadymmarkov left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks @guilhermearaujo 👏

@vadymmarkov vadymmarkov merged commit dbea29c into vadymmarkov:master Dec 7, 2018
@guilhermearaujo guilhermearaujo deleted the decodable branch December 7, 2018 16:46
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