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/fails on empty data for decodable #1508

Merged
merged 9 commits into from Dec 29, 2017

Conversation

Projects
None yet
6 participants
@Jeroenbb94

Jeroenbb94 commented Dec 15, 2017

Added support for allowing empty backend responses and returning the Decodable initialised if the coding key properties are optional.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 15, 2017

Codecov Report

Merging #1508 into development will decrease coverage by 0.04%.
The diff coverage is 81.81%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development   #1508      +/-   ##
==============================================
- Coverage        87.65%   87.6%   -0.05%     
==============================================
  Files               24      24              
  Lines             1053    1065      +12     
  Branches            96      97       +1     
==============================================
+ Hits               923     933      +10     
- Misses             128     130       +2     
  Partials             2       2
Impacted Files Coverage Δ
Sources/RxMoya/Observable+Response.swift 74.07% <100%> (ø) ⬆️
Sources/ReactiveMoya/SignalProducer+Response.swift 90.38% <100%> (ø) ⬆️
Sources/RxMoya/Single+Response.swift 100% <100%> (ø) ⬆️
Sources/Moya/Response.swift 88.13% <75%> (-0.55%) ⬇️

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 bbfd354...9d514ab. Read the comment docs.

codecov-io commented Dec 15, 2017

Codecov Report

Merging #1508 into development will decrease coverage by 0.04%.
The diff coverage is 81.81%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development   #1508      +/-   ##
==============================================
- Coverage        87.65%   87.6%   -0.05%     
==============================================
  Files               24      24              
  Lines             1053    1065      +12     
  Branches            96      97       +1     
==============================================
+ Hits               923     933      +10     
- Misses             128     130       +2     
  Partials             2       2
Impacted Files Coverage Δ
Sources/RxMoya/Observable+Response.swift 74.07% <100%> (ø) ⬆️
Sources/ReactiveMoya/SignalProducer+Response.swift 90.38% <100%> (ø) ⬆️
Sources/RxMoya/Single+Response.swift 100% <100%> (ø) ⬆️
Sources/Moya/Response.swift 88.13% <75%> (-0.55%) ⬇️

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 bbfd354...9d514ab. Read the comment docs.

@AndrewSB

This comment has been minimized.

Show comment
Hide comment
@AndrewSB

AndrewSB Dec 17, 2017

Member

Hey @Jeroenbb9! Thanks for doing this 🙃

Before we can accept it, can you please

  • Clean up the commits so SD10 & sunshinejr’s commits don’t appear as part of this PR
  • Fix the swiftlint warnings

🙌

Member

AndrewSB commented Dec 17, 2017

Hey @Jeroenbb9! Thanks for doing this 🙃

Before we can accept it, can you please

  • Clean up the commits so SD10 & sunshinejr’s commits don’t appear as part of this PR
  • Fix the swiftlint warnings

🙌

Show outdated Hide outdated Changelog.md Outdated
@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Dec 20, 2017

Member

@Jeroenbb94 Thanks for making that change. Would you mind rebasing to remove the commits that are not from you?

Member

SD10 commented Dec 20, 2017

@Jeroenbb94 Thanks for making that change. Would you mind rebasing to remove the commits that are not from you?

@Jeroenbb94

This comment has been minimized.

Show comment
Hide comment
@Jeroenbb94

Jeroenbb94 commented Dec 20, 2017

Done :)

@@ -1,6 +1,7 @@
# Next
### Added
- **Breaking Change** Added a `.requestCustomJSONEncodable` case to `Task`. [#1443](https://github.com/Moya/Moya/pull/1443) by [@evgeny-sureev](https://github.com/evgeny-sureev).
- **Breaking Change** Added `failsOnEmptyData` boolean support for the `Decodable` map functions. [#1508](https://github.com/Moya/Moya/pull/1508) by [@jeroenbb94](https://github.com/Jeroenbb94).

This comment has been minimized.

@sunshinejr

sunshinejr Dec 20, 2017

Member

I know this isn't the main point of this PR, but is it really a breaking change? There is a default argument for this parameter, this parameter is a last one, and if you don't provide an option you will get the same behavior as in previous versions. Or am I missing something? 🤔

@sunshinejr

sunshinejr Dec 20, 2017

Member

I know this isn't the main point of this PR, but is it really a breaking change? There is a default argument for this parameter, this parameter is a last one, and if you don't provide an option you will get the same behavior as in previous versions. Or am I missing something? 🤔

This comment has been minimized.

@SD10

SD10 Dec 20, 2017

Member

Sorry @sunshinejr that was me 😅
#1508 (comment)

@SD10

SD10 Dec 20, 2017

Member

Sorry @sunshinejr that was me 😅
#1508 (comment)

This comment has been minimized.

@SD10

SD10 Dec 20, 2017

Member

My definition of breaking is anything that will prevent the project from compiling. Changes in variable names, type names, function signatures, adding/removing cases from an enum, or changes to a more restrictive form of access control.

@SD10

SD10 Dec 20, 2017

Member

My definition of breaking is anything that will prevent the project from compiling. Changes in variable names, type names, function signatures, adding/removing cases from an enum, or changes to a more restrictive form of access control.

This comment has been minimized.

@sunshinejr

sunshinejr Dec 20, 2017

Member

Oh! You are absolutely correct @SD10, I just don't see any issue that could break it, do you have anything particular in mind?

@sunshinejr

sunshinejr Dec 20, 2017

Member

Oh! You are absolutely correct @SD10, I just don't see any issue that could break it, do you have anything particular in mind?

This comment has been minimized.

@SD10

SD10 Dec 20, 2017

Member

Nothing in particular, I just go by those general rules 😅 Someone could be relying on the function signature of Response.map. Using it as an argument of a higher order function maybe?

@SD10

SD10 Dec 20, 2017

Member

Nothing in particular, I just go by those general rules 😅 Someone could be relying on the function signature of Response.map. Using it as an argument of a higher order function maybe?

@SD10

SD10 approved these changes Dec 28, 2017

This one looks good to me. I'll wait a day to see if anyone else wants to review, then let it ⛵️

@SD10 SD10 merged commit c9ddcb3 into Moya:development Dec 29, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Dec 29, 2017

Member

Thanks a lot for contributing to Moya! I've invited you to join the Moya GitHub organization – no pressure to accept! If you'd like more information on what that means, check out our contributor guidelines and feel free to reach out with any questions.

Member

ashfurrow commented Dec 29, 2017

Thanks a lot for contributing to Moya! I've invited you to join the Moya GitHub organization – no pressure to accept! If you'd like more information on what that means, check out our contributor guidelines and feel free to reach out with any questions.

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Dec 29, 2017

Member

I'm going to tag #1357 because this requires some updates to the Chinese documentation

Member

SD10 commented Dec 29, 2017

I'm going to tag #1357 because this requires some updates to the Chinese documentation

@BasThomas BasThomas referenced this pull request Dec 29, 2017

Open

Changes in English docs not reflected in Chinese docs #1357

6 of 13 tasks complete

@Jeroenbb94 Jeroenbb94 deleted the Jeroenbb94:feature/failsOnEmptyData branch Dec 29, 2017

@SD10 SD10 added this to the 11.0 milestone Jan 11, 2018

@SD10 SD10 referenced this pull request Feb 8, 2018

Merged

[Release] Moya 11.0 #1568

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