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

Feature/#98 add spm support #101

Merged
merged 7 commits into from
Feb 5, 2020

Conversation

lvnkmn
Copy link
Collaborator

@lvnkmn lvnkmn commented Jan 29, 2020

Like mentioned in PR #97 I've created a simplified version of the same.

I've tested it and it works.
In order for the project to build I've also had to change the way the default URL handler works. This will cause the user to implement url handling in the app delegate in some cases and is therefore a breaking change and new version will therefore be 10.0.0.

I'll await results from Travis update it's stack if needed.

If all is well at some point. I can do two things:

  1. Merge and create a SPM only release tagged 10.0.0
    (since there's no new functionality for cocoapods users, I don't think this should be a problem)
  2. Merge and create a SPM only release tagged 10.0.0.prerelease

@Basca what's would be your preference in how to handle the release?

Menno Lovink added 3 commits January 29, 2020 15:05
…nURL is unavailable in application extensions for iOS`

note: this is a breaking change since it requires `open(_:open:options)` to be implemented in app delegate
@lvnkmn lvnkmn requested a review from Basca January 29, 2020 14:30
@lvnkmn lvnkmn mentioned this pull request Jan 29, 2020
@lvnkmn
Copy link
Collaborator Author

lvnkmn commented Jan 29, 2020

Awesome, tests succeed and checks have passed. @Basca are you ok with me merging this?

@SMillerDev
Copy link
Contributor

This doesn't test the swift build though.

@lvnkmn
Copy link
Collaborator Author

lvnkmn commented Jan 29, 2020

@SMillerDev can you elaborate? In most if not all SPM repo's I've seen this is the way to build. I personally don't see the benefit of changing travis's build setup for this currently.

It's testing the same code, and the SPM part is tested by just using it.

@lvnkmn
Copy link
Collaborator Author

lvnkmn commented Jan 29, 2020

Still not sure what you mean by "test the swift build" by the way. Because it actually does test the build:
image
and the only code to build is swift.

@SMillerDev
Copy link
Contributor

Right now the SPM integration is only tested when people use it, which will mean that you only know if it works if people report bugs. While you could also have it tested on every commit, since that's what travis-ci is for.

@lvnkmn
Copy link
Collaborator Author

lvnkmn commented Jan 29, 2020

@SMillerDev while you're technically correct, the same counts for issues caused within the 74.5 percent of the code that is not covered by the tests.

From a pragmatic standpoint I'd personally advice against trying to do what you're mentioning. If not, you're going to have to do this for both Cocoapods and SPM introducing less dry code, potentially having to write a new example project as well to fully cover what you're after.

Aside from that, the swift build command you have been using, builds for Mac OS and is therefore not a valid test either.

TLDR; trying to do this for iOS is likely to cause more harm than good, the SPM layer is quite thin, and already tested manually, also there's no changes required on a per release basis.

@SMillerDev
Copy link
Contributor

It's already done for cocoapods and the command I used was modified to work for iOS so it's a small but valid test. It just failed due to the iOS version targeted.

And if you split your code out good enough an argument can be made to test non of it since it's thin and already tested manually.

@lvnkmn lvnkmn force-pushed the feature/#98-Add-spm-support branch from 6d8ceea to 21c707d Compare January 29, 2020 16:38
@lvnkmn
Copy link
Collaborator Author

lvnkmn commented Jan 29, 2020

🎉
image

Allright, it turned out to be easier than I thought. I see no objections in merging this now. @Basca ?

@lvnkmn
Copy link
Collaborator Author

lvnkmn commented Jan 29, 2020

As for the commits mentioning (temporary):

  • pod repo update wasn't needed since all dependencies are known to travis.
  • the specs repo did not have to be explicitly added to the podfile

Both commits make the build proces faster.

Copy link
Contributor

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

Looks good to me other than this

.gitignore Show resolved Hide resolved
@lvnkmn
Copy link
Collaborator Author

lvnkmn commented Feb 5, 2020

Like discussed with @Basca will merge and create a SPM only release 10.0.0

@lvnkmn lvnkmn merged commit 9318f7b into M2mobi:master Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants