-
Notifications
You must be signed in to change notification settings - Fork 58
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
feat: add support for Swift Package Manager #15
Conversation
52000f1
to
d4dd12b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except the #if trick, seems mainly OK to me
But I saw in Slack that you had to do some other changes? And I've only reviewed that in the GitHub web interface and not been able to test it in practice yet, so I'll let you test it in practice and merge once ready.
import Stencil | ||
|
||
#if os(Linux) | ||
#if swift(>=3.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed #if swift(<3.1)
isn't a valid syntax… But I think #if ! swift(>=3.1)
is. This would avoid an empty #if
body and the main logic in the #else
You could even probably just write #if os(Linux) && ! swift(>=3.1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the spm branch I made, is esentially this + tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a branch that is based on @krzysztofzablocki 's feature/spm-support
branch of this PR or a separate branch from develop
? (Sorry I'm in the train, not enough connection for git fetch to work reliably to check myself 😄)
@djbe This branch is on the repo (and not a fork), and you both have ( @krzysztofzablocki + you) push access on this repo, so you should be able to add commits to this branch instead of creating a separate one, any reason not to here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only because I was playing around and wanted to let him compare. I'll delete mine and push the changes here, gimme a moment while I rebase the rest of the branches (so many branches 😆)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth adding swift build
/test to avoid regression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dunno, Rakefile (Xcode project) and swift package test the same thing. Testing both would just double the length of the CI tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its important because dependency bindings between podspec and spm are different, you might end up in situation when we update podspec dependency and forget SPM one and swift test / build would prevent it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add it to the rakefile? swift build && swift test
My rake test
is broken for some reason (I really really really hate ruby version shizzle)
6ac945a
to
d50fc84
Compare
d50fc84
to
c630580
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would xcpretty
work with SPM's output?
not afaik |
can we merge this? |
Ads suport for Swift Package Manager