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

enable spm #7

Merged
merged 2 commits into from
Dec 20, 2017
Merged

enable spm #7

merged 2 commits into from
Dec 20, 2017

Conversation

quanvo87
Copy link
Owner

@quanvo87 quanvo87 commented Dec 20, 2017

i changed the file structure to make it easy to enable SPM. this also resulted in:

  • updated Podspec, not sure if correct
  • travis originally failed, so I made the changes you see. They seem to run the test and pass...do you know the diff between what you did and what I put?

@quanvo87
Copy link
Owner Author

the only real changes were the podspec and travis. everything else is a change because its location was moved.

Copy link
Collaborator

@dingwilson dingwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall everything looks great. Left a quick point about testing re: .travis.yml. Not sure how we should proceed/what we should test.

PS: might want to gitignore .DS_Store

- swift build
Copy link
Collaborator

@dingwilson dingwilson Dec 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change tests for mac Swift 4 compatibility, and removes iOS 8-11 testing.

  1. We've proven that existing code is compatible with iOS 8-11, and so we can remove these tests. However, it might be a good idea to keep iOS testing for potential future changes. What do you think?

OTOH the current iOS tests run so slowly... 👎

  1. Do we want to do linux Swift 4 testing? I'm not sure if this is necessary, as we don't use any libraries (i.e. foundation).

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between swift 4 and iOS 8-11? If it's important then yea let's keep testing for them even though they're slow.

I think it could be worth it to test on Linux. Then we would be adding Linux compatibility. It's currently not. I think what this would take is writing a script that installs swift 4.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that testing for Swift 4 implies support for iOS 8-11. I think it should probably be fine if we don't explicitly test individual versions of iOS.

Yes, I think we would have to install Swift toolchains on a Linux env.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds good. I'll make Linux support a separate issue for later.

@quanvo87
Copy link
Owner Author

I'll add ds_store to gitignore!

The ds_store related files currently checked in to the repo can be safely removed, right?

@dingwilson
Copy link
Collaborator

Yes, you should be able to remove the .DS_Store files.

@quanvo87
Copy link
Owner Author

Once travis is done can you merge? Let's see if this broke cocoapods

Copy link
Collaborator

@dingwilson dingwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@quanvo87 quanvo87 merged commit a1bb0d3 into master Dec 20, 2017
@quanvo87 quanvo87 deleted the spm branch December 20, 2017 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants