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

Add framework project for Carthage compatibility #5

Merged
merged 6 commits into from Jun 5, 2017

Conversation

Jeehut
Copy link
Contributor

@Jeehut Jeehut commented May 2, 2017

This adds Carthage compatibility and therefore fixes #4.
I also added a section in the README. Note that a new release must be made until a stable version of this library can be installed via Carthage the way explained in the README (the recommended way).

Unfortunately I had to change the import statements in the files for MarqueeLabel since the guys over there have this issue around. Until it is fixed I suggest you merge this into a branch named "carthage" (or alike) and update the README to state: github "Daltron/NotificationBanner" "carthage" instead of the variant with the version at the end.

@Jeehut
Copy link
Contributor Author

Jeehut commented May 2, 2017

Just as a general hint: It's always good to prevent adding framework dependencies in frameworks if any possible. I'm not sure for what you're using SnapKit (haven's looked it up) but MarqueeLabel could definitely be removed for most users as not everybody needs a scolling label. Best practice would be to provide a basic library (that I would use) without it and to provide an extension to your library which also supports MarqueeLabel (as the guys have done with Alamofire for example – there are many extensions to it).

@Daltron
Copy link
Owner

Daltron commented May 3, 2017

Hi @Dschee! 😄 Thanks for taking the time to create a PR for this. I agree and am well aware not to add dependencies if they are not needed. SnapKit is an extremely popular auto layout DSL that could probably be removed if really desired but then the constraints code would be a little bit messier. However, the MarqueeLabel is definitely going to stay. You may not need it but I'm sure there are a lot of people who would benefit from it and I happen to be one of those people. 😄 I am also unsure on the current status of this PR? Does it actually work with Carthage? Your comments seem to indicate that it is not yet ready. I checked the status of the issue you mentioned above and the author seemed to indicate that it should work as expected.

@Jeehut
Copy link
Contributor Author

Jeehut commented May 10, 2017

Hi @Daltron and thank you for answering my questions.

First I'd like to say that I'm using SnapKit myself and think it should be included into this library if it's used in several places. So a 👍 for that. Just make sure not to set it to a too specific version so others can update their projects. My suggestion is instead of '~> 3.2.0' within the podspec file use '~> 3.2' which allows minor version updates (like 3.2 to 3.3). Those should be compatible with older versions according to SemVer 2.

As for MarqueeLabel, the main reason I reacted immediately with "don't include it if not necessary" is because it is not ready for framework inclusion yet – at least the Swift version isn't. See this issue where I report this in detail.

And to give clear answers to your question: Yes, this PR does work with Carthage for 100%! We've already released an app where we include NotificationBanner via my PR using Carthage, so that's proof enough I guess.

But I'm not sure if CocoaPods users will still be able to include NotificationBanner into their projects once this is merged. I'm not using CocoaPods anymore so I can't tell for sure but AFAIK the import MarqueeLabelSwift statement might give an error for CocoaPods users (since the CocoaPods dynamic framework is named MarqueeLabel only, without the Swift at the end). But maybe that's not a problem at all. If it is, you'd have to wait until the issue is fixed on the side of MarqueeLabel and merge this after that is done.

@cbpowell
Copy link
Contributor

cbpowell commented Jun 3, 2017

It's been awhile, so I thought I'd check in again - @Dschee @Daltron, you guys have any thoughts on my proposed solution here?

Select MarqueeLabel import based on build system
@Jeehut
Copy link
Contributor Author

Jeehut commented Jun 3, 2017

I think that might work, thanks for reminding about this again. Just merged it! 🎉

@Daltron
Copy link
Owner

Daltron commented Jun 3, 2017

@cbpowell Thanks for the follow up. @Dschee I will merge as soon as Carthage targets the 1.2 release. Is there anything special that needs to be done on my end? I'm unfamiliar with Carthage so any help would be appreciated!

@Jeehut
Copy link
Contributor Author

Jeehut commented Jun 5, 2017

@Daltron Nothing required to be done on your side, except for if you add new files to your code in the future – then they should of course be added to the project I created as well. Other than that, I think we set everything up already. What you could do (but is not required) is add this badge to the README to state that the project is Carthage compatible.

You also don't need to do anything special when releasing new versions when using Carthage by the way. You just need to tag the version in git (/on GitHub) as you already have done in the past – Carthage will use the tags as versions. No special file like the .podspec from CocoaPods needed.

@Daltron Daltron merged commit 6003b41 into Daltron:master Jun 5, 2017
@Daltron
Copy link
Owner

Daltron commented Jun 5, 2017

A Carthage compatible badge has been added to the README. Thanks @Dschee and @cbpowell for all your help! Its greatly appreciated! 😄 👍

Closes #5

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.

Carthage Compatiblity
3 participants