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

Replace TargetType validate: Bool with validationType: ValidationType #1505

Merged
merged 9 commits into from Dec 28, 2017

Conversation

Projects
None yet
4 participants
@SD10
Member

SD10 commented Dec 12, 2017

This resolves #1447 and completes #1454 which was started by @amaurydavid

It replaces the Boolean property of TargetType with a ValidationType enum allowing the user to customize the status codes used by Alamofire's validate(statusCodes:) method.

TODO:

  • CHANGELOG
  • Update Documentation
  • Update Example app
@MoyaBot

This comment has been minimized.

Show comment
Hide comment
@MoyaBot

MoyaBot Dec 12, 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 318 lines
Observable+MoyaSpec.swift 492 File should contain 400 lines or less: currently contains 492
SignalProducer+MoyaSpec.swift 10 Type body should span 200 lines or less excluding comments and whitespace: currently spans 302 lines
SignalProducer+MoyaSpec.swift 474 File should contain 400 lines or less: currently contains 474
Single+MoyaSpec.swift 6 Type body should span 200 lines or less excluding comments and whitespace: currently spans 306 lines
Single+MoyaSpec.swift 484 File should contain 400 lines or less: currently contains 484
TestHelpers.swift 55 Operators should be surrounded by a single whitespace when defining them.

Generated by 🚫 Danger

MoyaBot commented Dec 12, 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 318 lines
Observable+MoyaSpec.swift 492 File should contain 400 lines or less: currently contains 492
SignalProducer+MoyaSpec.swift 10 Type body should span 200 lines or less excluding comments and whitespace: currently spans 302 lines
SignalProducer+MoyaSpec.swift 474 File should contain 400 lines or less: currently contains 474
Single+MoyaSpec.swift 6 Type body should span 200 lines or less excluding comments and whitespace: currently spans 306 lines
Single+MoyaSpec.swift 484 File should contain 400 lines or less: currently contains 484
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 Dec 18, 2017

Codecov Report

Merging #1505 into development will decrease coverage by 0.67%.
The diff coverage is 71.42%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1505      +/-   ##
===============================================
- Coverage        87.65%   86.98%   -0.68%     
===============================================
  Files               24       25       +1     
  Lines             1053     1083      +30     
  Branches            96       98       +2     
===============================================
+ Hits               923      942      +19     
- Misses             128      139      +11     
  Partials             2        2
Impacted Files Coverage Δ
Sources/Moya/TargetType.swift 50% <100%> (-50%) ⬇️
Sources/Moya/MultiTarget.swift 100% <100%> (ø) ⬆️
Sources/Moya/MoyaProvider+Internal.swift 92.82% <66.66%> (+0.09%) ⬆️
Sources/Moya/ValidationType.swift 66.66% <66.66%> (ø)

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...93eea52. Read the comment docs.

codecov-io commented Dec 18, 2017

Codecov Report

Merging #1505 into development will decrease coverage by 0.67%.
The diff coverage is 71.42%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1505      +/-   ##
===============================================
- Coverage        87.65%   86.98%   -0.68%     
===============================================
  Files               24       25       +1     
  Lines             1053     1083      +30     
  Branches            96       98       +2     
===============================================
+ Hits               923      942      +19     
- Misses             128      139      +11     
  Partials             2        2
Impacted Files Coverage Δ
Sources/Moya/TargetType.swift 50% <100%> (-50%) ⬇️
Sources/Moya/MultiTarget.swift 100% <100%> (ø) ⬆️
Sources/Moya/MoyaProvider+Internal.swift 92.82% <66.66%> (+0.09%) ⬆️
Sources/Moya/ValidationType.swift 66.66% <66.66%> (ø)

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...93eea52. Read the comment docs.

@sunshinejr

Looks good! Thanks for the work @SD10. Would you be up for updating the documentation and checking the Ci error? Oh, and nice work on static dispatch in our tests 😉

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Dec 18, 2017

Member

@sunshinejr I will update the docs tonight and start a Moya 12.0 migration guide. I'm not sure about the CI error, right now that specific test won't pass locally but CI runs fine. I guess I'll just use CI to run my tests for now. Curious if others face this issue on their local copy as well

Member

SD10 commented Dec 18, 2017

@sunshinejr I will update the docs tonight and start a Moya 12.0 migration guide. I'm not sure about the CI error, right now that specific test won't pass locally but CI runs fine. I guess I'll just use CI to run my tests for now. Curious if others face this issue on their local copy as well

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Dec 19, 2017

Member

@SD10 which test didn't pass locally for you? There are few tests that should be separated, I talk about it at #1424, basically some of the test fail under bad internet connection.

Member

sunshinejr commented Dec 19, 2017

@SD10 which test didn't pass locally for you? There are few tests that should be separated, I talk about it at #1424, basically some of the test fail under bad internet connection.

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Dec 19, 2017

Member

Ok I've completed the following:

  • Updated the example app
  • Added a deprecation warning for validate: Bool because I noticed this change fails silently
  • Updated the docs, only AlamofireValidation.md seemed to be affected
Member

SD10 commented Dec 19, 2017

Ok I've completed the following:

  • Updated the example app
  • Added a deprecation warning for validate: Bool because I noticed this change fails silently
  • Updated the docs, only AlamofireValidation.md seemed to be affected

@SD10 SD10 merged commit 0faad7f into development Dec 28, 2017

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Dec 28, 2017

Member

I'm tagging #1357 because AlamofireValidation.md will need to be updated in the Chinese documentation.

Member

SD10 commented Dec 28, 2017

I'm tagging #1357 because AlamofireValidation.md will need to be updated in the Chinese documentation.

@SD10 SD10 deleted the enhancement/validation branch Dec 28, 2017

@BasThomas BasThomas referenced this pull request Dec 28, 2017

Open

Changes in English docs not reflected in Chinese docs #1357

6 of 12 tasks complete

@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