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

require extension-safe api for RxMoya and ReactiveMoya #1417

Merged
merged 9 commits into from Nov 13, 2017

Conversation

Projects
None yet
7 participants
@spookyvision
Contributor

spookyvision commented Oct 26, 2017

augmenting #1416 to include ReactiveMoya target as well

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 26, 2017

Codecov Report

Merging #1417 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1417   +/-   ##
=======================================
  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 025548b...22501f0. Read the comment docs.

codecov-io commented Oct 26, 2017

Codecov Report

Merging #1417 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1417   +/-   ##
=======================================
  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 025548b...22501f0. Read the comment docs.

@BasThomas

We should not forget adding a Changelog entry for this one by the way 👍

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 28, 2017

Member

Yup, can you add a Changelog entry for this one, @spookyvision? Other than that it looks great.

About the Changelog update - you should rebase master before you add this one, as we already have one fix in it. This entry should be under Fixed column, as we enabled this flag before, but not for each target for both debug and release.

Member

sunshinejr commented Oct 28, 2017

Yup, can you add a Changelog entry for this one, @spookyvision? Other than that it looks great.

About the Changelog update - you should rebase master before you add this one, as we already have one fix in it. This entry should be under Fixed column, as we enabled this flag before, but not for each target for both debug and release.

@spookyvision

This comment has been minimized.

Show comment
Hide comment
@spookyvision

spookyvision Nov 6, 2017

Contributor

First time doing this, hope I did things right :)

Contributor

spookyvision commented Nov 6, 2017

First time doing this, hope I did things right :)

@AndrewSB

This comment has been minimized.

Show comment
Hide comment
@AndrewSB

AndrewSB Nov 6, 2017

Member

@spookyvision it's a little worrying to see 25 commits in this PR. Any idea why your PR is pulling in so many other commits?

Member

AndrewSB commented Nov 6, 2017

@spookyvision it's a little worrying to see 25 commits in this PR. Any idea why your PR is pulling in so many other commits?

@spookyvision

This comment has been minimized.

Show comment
Hide comment
@spookyvision

spookyvision Nov 6, 2017

Contributor

@AndrewSB I rebased onto master as requested by @sunshinejr - my commits are actually far less. Might have messed up there. Should I start from scratch and cherrypick them?

Contributor

spookyvision commented Nov 6, 2017

@AndrewSB I rebased onto master as requested by @sunshinejr - my commits are actually far less. Might have messed up there. Should I start from scratch and cherrypick them?

@SD10 SD10 changed the base branch from master to development Nov 13, 2017

@SD10 SD10 changed the base branch from development to master Nov 13, 2017

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Nov 13, 2017

Member

I just did a roundtrip swap on the target branch of this PR and it seemed to remove a lot of the excess commits in this PR. I don't know if we want to squash the commits further but the changes LGTM

Member

SD10 commented Nov 13, 2017

I just did a roundtrip swap on the target branch of this PR and it seemed to remove a lot of the excess commits in this PR. I don't know if we want to squash the commits further but the changes LGTM

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Nov 13, 2017

Member

I've resolved conflicts, it seems ready to merge! Thanks @spookyvision! 🎉

Member

sunshinejr commented Nov 13, 2017

I've resolved conflicts, it seems ready to merge! Thanks @spookyvision! 🎉

@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Nov 13, 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 Nov 13, 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.

@sunshinejr sunshinejr merged commit f378b06 into Moya:master Nov 13, 2017

1 check was pending

ci/circleci CircleCI is running your tests
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment