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

Combine support! #1904

Merged
merged 33 commits into from Sep 6, 2019

Conversation

@sunshinejr
Copy link
Member

commented Sep 2, 2019

Fixes #1871

@MoyaBot

This comment has been minimized.

Copy link

commented Sep 2, 2019

Fails
🚫

danger-swift` failed.

Log

Cloning and building inline dependencies: import DangerSwiftProse // package: https://github.com/f-meloni/danger-swift-prose.git, import DangerXCodeSummary // package: https://github.com/f-meloni/danger-swift-xcodesummary.git, this might take some time.
�[0;0m�[31mERROR: 💥  Failed to compile script
�[0;0m

Generated by 🚫 dangerJS against 8b3939f

@codecov-io

This comment has been minimized.

Copy link

commented Sep 2, 2019

Codecov Report

Merging #1904 into development will increase coverage by 1.18%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1904      +/-   ##
===============================================
+ Coverage        92.48%   93.66%   +1.18%     
===============================================
  Files               26       26              
  Lines              932      932              
===============================================
+ Hits               862      873      +11     
+ Misses              70       59      -11
Impacted Files Coverage Δ
Sources/Moya/Response.swift 95.4% <0%> (+4.59%) ⬆️
Sources/RxMoya/Observable+Response.swift 61.53% <0%> (+7.69%) ⬆️
Sources/ReactiveMoya/SignalProducer+Response.swift 93.93% <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 bf503d5...8b3939f. Read the comment docs.

@sunshinejr sunshinejr referenced this pull request Sep 3, 2019

@sunshinejr sunshinejr changed the title [WIP] Combine support! Combine support! Sep 4, 2019

@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

@Moya/contributors anyone able to make a review for this one? 🙏

@LucianoPAlmeida
Copy link
Member

left a comment

Awesome 💯

Ohh there's a test crashing on the macOS in the CI

@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

Seems like tests on macOS are crashing because the CircleCI environment has macOS Mojave, not Catalina? There is this @available(OSX 10.15...) so it shouldn't run but let me check this one.

@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

Okay it built, but Danger seems to have troubles (currently xcode-summary plugin). cc @f-meloni

@f-meloni

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

:O today I will have a look, also because I don't see a specific reason for it to fail, it looks Marathon that is having problem to clone them

@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

@f-meloni no rush!

@f-meloni

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Could be Xcode 11.0 the problem 🤔 Will try to compile them with Xcode 11 and see what happens

@f-meloni

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Found it!

<unknown>:0: error: invalid value '5.1' in '-swift-version 5.1'
<unknown>:0: note: valid arguments to '-swift-version' are '4', '4.2', '5'

Marathon sets 5.1 as version, but apparently that is not recognised as valid argument from swiftc yet.
Now this is the problem... Will have a thinking on how solve it, before proposing SPM + Rocket, that is a possibility and would remove the need of using Marathon

@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

@f-meloni I think 5.1 is just specified as 5 but depending on Xcode it will use either 5.0 or 5.1 so they probably won't add 5.1 as a valid argument. 5.1 seems to be valid argument only for SPM tools version for the moment.

I'm gonna merge this PR for now so we have one valid cache for the PRs. If anyone finds something in this PR don't hesitate to comment/open a PR/open an issue!

@sunshinejr sunshinejr merged commit dcd5cc0 into development Sep 6, 2019

1 of 2 checks passed

ci/circleci: build Your tests failed on CircleCI
Details
ci/circleci: carthage_without_swiftlint_integration Your tests passed on CircleCI!
Details

@sunshinejr sunshinejr deleted the feature/combine branch Sep 6, 2019

@f-meloni

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

@sunshinejr It accepts '4', '4.2', then I would expect it to accept '5', '5.1' too :thinking:

@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

@f-meloni true, but 4.1 was still 4 in terms of swift version, I think the 4.2 is because there were a lot of breaking changes and it should’ve been named 5.0 but they couldn’t because the ABI wasn’t stable. So in a sense these are all just major releases.

@f-meloni

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

@sunshinejr Yeah you probably are right!

@f-meloni

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

Added a fix to Marathon that should solve the problem

JohnSundell/Marathon#209

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