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

Set macOS deployment target to 10.10 to follow pospec #1444

Merged
merged 3 commits into from Nov 9, 2017

Conversation

Projects
None yet
8 participants
@lucas34
Member

lucas34 commented Nov 6, 2017

Link to issue #1441
Solve macOS deployment target discrepancy when using Carthage or pod.

I updated for all target. Moya, MoyaTest, RxMoya and ReactiveMoya.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 6, 2017

Codecov Report

Merging #1444 into master will increase coverage by 5.48%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1444      +/-   ##
==========================================
+ Coverage   82.92%   88.41%   +5.48%     
==========================================
  Files           5        5              
  Lines         164      164              
==========================================
+ Hits          136      145       +9     
+ Misses         28       19       -9
Impacted Files Coverage Δ
Sources/RxMoya/Observable+Response.swift 70.58% <0%> (+8.82%) ⬆️
Sources/ReactiveMoya/SignalProducer+Response.swift 90.9% <0%> (+9.09%) ⬆️
Sources/RxMoya/Single+Response.swift 100% <0%> (+12.5%) ⬆️

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 e4eeb7e...54ffb0d. Read the comment docs.

codecov-io commented Nov 6, 2017

Codecov Report

Merging #1444 into master will increase coverage by 5.48%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1444      +/-   ##
==========================================
+ Coverage   82.92%   88.41%   +5.48%     
==========================================
  Files           5        5              
  Lines         164      164              
==========================================
+ Hits          136      145       +9     
+ Misses         28       19       -9
Impacted Files Coverage Δ
Sources/RxMoya/Observable+Response.swift 70.58% <0%> (+8.82%) ⬆️
Sources/ReactiveMoya/SignalProducer+Response.swift 90.9% <0%> (+9.09%) ⬆️
Sources/RxMoya/Single+Response.swift 100% <0%> (+12.5%) ⬆️

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 e4eeb7e...54ffb0d. Read the comment docs.

@SD10

SD10 approved these changes Nov 6, 2017

LGTM. I'm just not sure if the podspec was meant to be upgraded to 10.11 or this downgraded

@BasThomas

Looks good to me. Thanks, @lucas34!

@BasThomas

This comment has been minimized.

Show comment
Hide comment
@BasThomas

BasThomas Nov 6, 2017

Contributor

@SD10 according to #1441 (comment):

our podspec already uses 10.10, thus I believe that this is a bug that our Moya.xcodeproj uses 10.11 as a deployment target and it should be safe to fix.

Contributor

BasThomas commented Nov 6, 2017

@SD10 according to #1441 (comment):

our podspec already uses 10.10, thus I believe that this is a bug that our Moya.xcodeproj uses 10.11 as a deployment target and it should be safe to fix.

@sunshinejr

sunshinejr requested changes Nov 6, 2017 edited

Looks good, @lucas34, thanks for doing that - looks good! Can you also add a Changelog entry that we fixed Carthage macOS deployment target, from 10.11 to 10.10? 😉 It should be under Fixed column, which is already created in Changelog.md.

@AndrewSB

Can you remove your merge commit from this Pull Request please? Just the one with the change please

@lucas34

This comment has been minimized.

Show comment
Hide comment
@lucas34

lucas34 Nov 7, 2017

Member

Sure, I removed the merge commit

Member

lucas34 commented Nov 7, 2017

Sure, I removed the merge commit

@pedrovereza

💯

Show outdated Hide outdated Changelog.md Outdated

lucas34 and others added some commits Nov 6, 2017

@lucas34

This comment has been minimized.

Show comment
Hide comment
@lucas34

lucas34 Nov 8, 2017

Member

Looks like @BasThomas added the merge commit back. Is it ok ?

Member

lucas34 commented Nov 8, 2017

Looks like @BasThomas added the merge commit back. Is it ok ?

@AndrewSB

This comment has been minimized.

Show comment
Hide comment
@AndrewSB

AndrewSB Nov 9, 2017

Member

Yup, it looks great.

Oh interesting, the merge commits are from the GitHub CTA... I wonder if they show up in the history of master once we merged. I guess we're going to find out!

Member

AndrewSB commented Nov 9, 2017

Yup, it looks great.

Oh interesting, the merge commits are from the GitHub CTA... I wonder if they show up in the history of master once we merged. I guess we're going to find out!

@AndrewSB AndrewSB merged commit 1bdcb5c into Moya:master Nov 9, 2017

1 check was pending

ci/circleci CircleCI is running your tests
Details
@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

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

@AndrewSB

This comment has been minimized.

Show comment
Hide comment
@AndrewSB

AndrewSB Nov 9, 2017

Member

Wow, that's incredibly annoying
messages image 254709031

Github adds them as clean merges, complete noise.

Member

AndrewSB commented Nov 9, 2017

Wow, that's incredibly annoying
messages image 254709031

Github adds them as clean merges, complete noise.

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