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

Making range filter generic to any range type. #1624

Merged
merged 4 commits into from Apr 6, 2018

Conversation

Projects
None yet
5 participants
@LucianoPAlmeida
Copy link
Member

LucianoPAlmeida commented Apr 4, 2018

Hey guys :))
Proposal
I did a PR adding a convenience filter for Range #1605
But now I think is a better idea make instead of adding a convenience function for any Range type, just make the existing function generic and maintain only on function implementation that can handle any range type for a filter.

@MoyaBot

This comment has been minimized.

Copy link

MoyaBot commented Apr 4, 2018

2 Warnings
⚠️ The Cartfile or Cartfile.resolved was updated, but there were no changes in the podspec. Did you forget updating the podspec?
⚠️ The library files were changed, but the tests remained unmodified. Consider updating or adding to the tests to match the library changes.

Generated by 🚫 Danger

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 4, 2018

Codecov Report

Merging #1624 into development will decrease coverage by 0.66%.
The diff coverage is 100%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1624      +/-   ##
===============================================
- Coverage        88.48%   87.82%   -0.67%     
===============================================
  Files                5        5              
  Lines              165      156       -9     
===============================================
- Hits               146      137       -9     
  Misses              19       19
Impacted Files Coverage Δ
Sources/RxMoya/Single+Response.swift 100% <100%> (ø) ⬆️
Sources/ReactiveMoya/SignalProducer+Response.swift 90.9% <100%> (-0.76%) ⬇️
Sources/RxMoya/Observable+Response.swift 70.58% <100%> (-2.39%) ⬇️

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 3e50289...605b31f. Read the comment docs.

@sunshinejr

This comment has been minimized.

Copy link
Member

sunshinejr commented Apr 4, 2018

@LucianoPAlmeida just wanted to say that this is the first time I'm seeing RangeExpression type and I'm amazed. Will probably take a closer look on this and PR later on, but great job on finding it 💯

@LucianoPAlmeida

This comment has been minimized.

Copy link
Member Author

LucianoPAlmeida commented Apr 5, 2018

Hey @sunshinejr I felt the same way when I find out about it, I always amazed at these things on the Swift :))

@@ -48,24 +48,13 @@ public extension Response {
- statusCodes: The range of acceptable status codes.

This comment has been minimized.

@SD10

SD10 Apr 5, 2018

Member

May want to remove the reference to a closed range in the comment above

@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Apr 5, 2018

We'd also need a CHANGELOG entry, this is API breaking cuz we've changed signatures

LucianoPAlmeida and others added some commits Apr 5, 2018

@SD10

SD10 approved these changes Apr 5, 2018

Copy link
Member

SD10 left a comment

This is pretty nifty 👍 LGTM but I'm going to let another contributor sign off on this

@sunshinejr
Copy link
Member

sunshinejr left a comment

Awesome, thanks for taking care of this @LucianoPAlmeida! 💯

@sunshinejr sunshinejr merged commit af55d6d into Moya:development Apr 6, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@SD10 SD10 referenced this pull request Nov 2, 2018

Merged

[Release] Moya 12.0 #1753

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.