search

CombineCommunity / CombineExt

CombineExt provides a collection of operators, publishers and utilities for Combine, that are not provided by Apple themselves, but are common in other Reactive Frameworks and standards.
https://combine.community
MIT License
1.72k stars 151 forks source link

Fixes for the xcodeproj file #133

Closed jasdeepsaini closed 2 years ago

jasdeepsaini commented 2 years ago

Fixes to get the project and tests to build:

Project cleanup

Missing library and test files added to the project:

freak4pc commented 2 years ago

I'll tryto find some time to review this - I'll start by saying I'd prefer keeping the deployment target low (iOS 10 or 11), because thjere could be some older codebases that don't necessarily use Combine all over the place but can still benefit from using CombineExt in some places that are guarded by availability clauses.

jasdeepsaini commented 2 years ago

I'll tryto find some time to review this - I'll start by saying I'd prefer keeping the deployment target low (iOS 10 or 11), because thjere could be some older codebases that don't necessarily use Combine all over the place but can still benefit from using CombineExt in some places that are guarded by availability clauses.

The deployment target is an issue if you want to use the latest version of Combine-Schedulers which requires iOS 13. With the lower deployment target, the library and unit tests won't build. The Package.swift file has a minimum iOS version of 13 so older projects won't be able to pull in this package.

freak4pc commented 2 years ago

I'll have to look. I'm not sure if there's a way to give a different deploymetn target to the test target.

freak4pc commented 2 years ago

Anyways if that's the case we can leave the old version of combine-schedulers, It's just a test utility so I'm not sure it matters too much.

jasdeepsaini commented 2 years ago

Anyways if that's the case we can leave the old version of combine-schedulers, It's just a test utility so I'm not sure it matters too much.

I reverted the deployment target of CombineExt to iOS 10.0 and changed the deployment target of CombineExtTests to iOS 13.0. Then I moved the CombineSchedulers package from CombineExt to CombineExtTests.

This should do what you wanted while allowing tests to be run if CombineExt is modified in Xcode instead of the command line.

freak4pc commented 2 years ago

LGTM. Let's just fix the conflicts :) (Or add DS_Store to .gitignore which would be even better)

jasdeepsaini commented 2 years ago

LGTM. Let's just fix the conflicts :) (Or add DS_Store to .gitignore which would be even better)

I added .DS_Store to .gitignore.

codecov[bot] commented 2 years ago

Codecov Report

Merging #133 (1cd51ba) into main (5c17b57) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 1cd51ba differs from pull request most recent head 72e610b. Consider uploading reports for the commit 72e610b to get more accurate results

@@           Coverage Diff           @@
##             main     #133   +/-   ##
=======================================
  Coverage   95.51%   95.51%           
=======================================
  Files          70       70           
  Lines        4166     4166           
=======================================
  Hits         3979     3979           
  Misses        187      187

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 5c17b57...72e610b. Read the comment docs.