-
Notifications
You must be signed in to change notification settings - Fork 350
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
Podfile Refactor #281
Podfile Refactor #281
Conversation
self.current_target_definition = definition | ||
yield | ||
inherit!(:complete) |
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.
shouldn't be necessary since its the default
🎉 |
3d0c08d
to
93db795
Compare
Preliminary docs added. @orta I'd greatly appreciate help making them more helpful for the guides |
Beyond the documentation bits, I think this PR is ready for @CocoaPods/core to review. |
I'll try out today but that's huge! |
it 'returns the installation method' do | ||
name, options = Podfile.new {}.installation_method | ||
name.should == 'cocoapods' | ||
options.should == {} |
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 like this format ( https://github.com/artsy/eigen/pull/1003/files ) - I think it's good and future-proof 👍
Might be the PR to remove these too:
|
# pod 'JSONKit' | ||
# end | ||
# end | ||
# | ||
# @return [void] | ||
# | ||
def target(name, options = {}) | ||
if options && !options.keys.all? { |key| [:exclusive].include?(key) } | ||
def target(name, options = nil) |
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.
why have options at all? if we raise if they get sent at all?
for people who passed options in the current CocoaPods version. I'll add a TODO to remove the parameter, though, in a few months (it's just to make upgrading friendlier)
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.
legit 👍
bfcccd5
to
e04ca18
Compare
Great work on this @segiddins, nicely cleaned up 👍 👏 |
This part of the podfile refactor is pretty much done.
Closes #151.