GitHawkApp / ContextMenu

An iOS context menu UI inspired by Things 3.
http://githawk.com/
MIT License
982 stars 51 forks source link

Run pod install on Example #41

Closed sbarow closed 6 years ago

sbarow commented 6 years ago

Example.xcworkspace is generated by pod install so there is no need to check it in. Removing this allows carthage install to work again with Swift 4.2

If Carthage detects that there is a .workspace then it will try build that instead of ContextMenu.xcodeproj.

rnystrom commented 6 years ago

Won’t this require a pod install to get examples working now? I’d like the repo to all work just by downloading.

Sent with GitHawk

sbarow commented 6 years ago

@rnystrom yeah it would, best practise is not to checkin .xcworkspace & Pods folders.

rnystrom commented 6 years ago

@sbarow any reason why? All of our repos in this org check in all assets so someone doesn’t need CocoaPods to get working. Projects are just there and working.

Why does this need removed for Carthage?

Sent with GitHawk

sbarow commented 6 years ago

Main reason is they can be generated, checking them in will lead to merge conflicts and other issues down the line, its an unnecessary pain point for engineers - this generally outweighs people having to have cocoapods pods installed and running pod install

I believe the issue why Carthage isn't able to build this is that its detecting a .xcworkspace and trying to build ContextMenu from the workspace scheme, the problem is when the 4.2 update was done I don't think pod install was run on the example directory, so the workspace is still technically building the 4.0 version.

Another reason not to checkin generated code ;-)

I can update the diff with a fix for the cocoa pods issue if thats how you'd prefer to keep it.

rnystrom commented 6 years ago

Ya if rather keep this repo inline with the others instead of introduce another maintenance model. If were gonna do this, wed want to do it to everything (and I’m not convinced it matters after maintaining large OSS work with checked-in Pods for 3 years).

IMO let’s just do the minimal fix to make Cartage work.

Sent with GitHawk

sbarow commented 6 years ago

Sounds good, just ran pod install & reverted all the other changes.

rnystrom commented 6 years ago

Thank you!

Sent with GitHawk

telip007 commented 5 years ago

Carthage still not working. Getting error code 65. @sbarow @rnystrom

telip007 commented 5 years ago

Ah sorry, used the latest release but it does not include this change :) So with this commit it works. 👍

rnystrom commented 5 years ago

Will update!

Sent with GitHawk