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 support for the specification of dependencies by configuration #52

Closed
wants to merge 2 commits into from

Conversation

nevyn
Copy link

@nevyn nevyn commented Dec 13, 2013

Podfile parsing of the functionality for CocoaPods/CocoaPods#1668 .

@irrationalfab has opinions on the syntax. I built what @alloy suggested. I think @alloy's syntax is easier for beginners (I'm thinking from the perspective of being the author of http://lookback.io/docs/install-using-cocoapods , where I want to make step #2 as short as possible). I also agree that it's weird to overload a hash whose primary usage is to specify where the code is, with linker settings. I think the usability argument is stronger, though. Both syntaxes could easily be supported.

I've written the code so it should be easy to switch to @irrationalfab's syntax, or to support both. Where do we go from here? (Also, what should I fix in my PR before it's mergeable? :) )

An option to only link a specific pod to the target when building in a specific configuration.
The exact syntax is still TBD, pending discussion in CocoaPods/CocoaPods#1668
which implements the corresponding functionality.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.14%) when pulling 296df85 on lookback:pods-by-config into 88f665e on CocoaPods:master.

@fabiopelosin
Copy link
Member

The point about my syntax was to create a Podfile::ConfigurationDefinition class which stores dependencies and inherits all the dependencies of the parent.

This is what I'm proposing in CocoaPods/CocoaPods#840 (comment) i.e. to make 3 classes each one which can have children definitions and inherits all the definitions of the parent:

  • Podfile::Definition::Project
  • Podfile::Definition::Target
  • Podfile::Definition::Configuration

Those classes should inherit from the Podfile::Definition which would be a (maybe) stripped down version of the current Podfile::TargetDefinition.

Later I will elaborate more on this.

@fabiopelosin
Copy link
Member

I'm thinking from the perspective of being the author of http://lookback.io/docs/install-using-cocoapods , where I want to make step #2 as short as possible

This is a very important perspective to take into account. However I would prefer to be explicit over implicit (the second step of your guide assumes that the user is looking to integrate the first target).

I think that discussing the intended syntax is important because if we deduce to take a different approach later on (like an inheritance based one like I'm proposing) we basically would need to unwind this patch.

Both syntaxes could easily be supported.

This affects affects the clarity of the syntax for end users and the maintainability/flexibility of the codebase. If you ask to me I would rework the current syntax but I'm not even considering it at the moment for backward compatibility and resources. For example it makes more sense to me do:

target '1' do
  pod 'MY_POD'
end

target '2' do
  pod 'MY_POD'
end

use_source 'MY_POD', :git => 'www.example.com'

instead of

target '1' do
  pod 'MY_POD', :git => 'www.example.com'
end

target '2' do
  pod 'MY_POD', :git => 'www.example.com'
end

Same goes for warnings inhibition, the :podspec and the :path options. Those features are orthogonal to the declaration of the dependencies of a target.

This point also highlights how sticky are the syntax choices for the Podfile.

@alloy I would love to hear your opinion.

@alloy
Copy link
Member

alloy commented Dec 13, 2013

Same goes for warnings inhibition, the :podspec and the :path options. Those features are orthogonal to the declaration of the dependencies of a target.

While you are correct, with regards to implementation details, this is a DSL and as such is intrinsically an application of the facade pattern. Therefore I want a Podfile like this to work:

target '1' do
  pod 'MY_POD', :git => 'www.example.com'
end

Internally the DSL should immediately separate those concerns, which is what it already does, which then also makes it possible to use the use_source directive if you would need to reference that pod in multiple targets.

I’m maintaining my ‘it should be possible in both ways’ stance, purely from a UX pov, of which I am convinced they like being able to specify it inline. Consider that probably most Podfiles out there will only have one or two pods, let alone multiple targets.

@fabiopelosin
Copy link
Member

While you are correct, with regards to implementation details, this is a DSL and as such is intrinsically an application of the facade pattern.

Fully agree! Nonetheless I would prefer, for maintenability reasons, to match the DSL with the implementation as we have full control of both of them and they represent the same concepts. Also I think that the implementation should use inheritance regardless of the decision for the DSL.

Therefore I want a Podfile like this to work: [...]

I don't agree on the therefore 😉. Jokes aside, I don't see the benefit of the alternative inline declaration as use_source offers the same convenience (without the need to change if you add a target or duplicate). Ok, actually I consider it less convenient by a non-significant amount.

I have some idealistic mantras in this regard:

  • two ways of doing the same thing are confusing for the users
  • we can offer convenience through other means
  • hiding complexity doesn't help, reducing it does
  • the DSL should not lie (why can I specify a dependency from one source and the same dependency from another?; does it fetches it from different sources?)

Considering the Lookback guide, and applying them, I think that the best solution would be:

With CocoaPods in place, you can now install LookBack. Create a file called Podfile with the following command:

$ pod init

After you can add the following statement to the debug configuration of you target:

pod 'Lookback'

Note that with this description there are no assumptions about the target and the solution offers the same grade of convenience (imo). Also note that pod init will have to create the definitions for the configurations but this is not an issue.

I’m maintaining my ‘it should be possible in both ways’ stance, purely from a UX pov, of which I am convinced they like being able to specify it inline.

I'm not big fan of the Perl way but I see your point.

Consider that probably most Podfiles out there will only have one or two pods, let alone multiple targets.

This argument is gained some ground with me and I consider it solid! CocoaPods – as Xcode – curse is that it is a developer tool so it should support all the reasonable configurations while still being as much convenient as possible, and everyone knows how things end up with two bosses.

@nevyn
Copy link
Author

nevyn commented Dec 16, 2013

Also I think that the implementation should use inheritance regardless of the decision for the DSL.

I have no opinion on whether your suggested structure makes sense, but I'd rather do such a restructuring separate from this feature (and I'd rather not do it). This patch is small and easy to rewrite; the important part is to stick the functionality in there so all us developer tool developers can ease our users' lives.

After you can add the following statement to the debug configuration of you target:

I really don't want to make my user go hunt documentation to figure out what that sentence means, nor reference abstract locations in scary text files. Right now, they can drag-and-drop a file into their project and they're done. I disagree that your suggestion is even nearly as convenient.

I'm making the assumption that most of my users have created an Xcode project from one of the templates, and stuck mostly with that for the lifetime of their project, and thus have only one target. For experienced developers on large projects that might not be the case, but they will have no problem adapting. (If I remember correctly I think I saw in a previous discussion that you have already decided to drop the default target in a future version, so maybe you've already had this discussion and disagree with the premise.)

The oneliner "pod 'Lookback', :configurations => ['Test']" is succinct and pragmatic in either case.

This argument is gained some ground with me and I consider it solid!

Does this mean you're okay with merging this patch and the one that depends on it?

@fabiopelosin
Copy link
Member

The important part is to stick the functionality in there so all us developer tool developers can ease our users' lives.

I would generally prefer to properly address the underlying architecture before addressing patches. However I see that this discussion is stalled, hence I will let the call to @alloy (i.e. I consider either way reasonable).

I really don't want to make my user go hunt documentation to figure out what that sentence mean [...]
nor reference abstract locations in scary text files

project 'MyProject.xcodeproj' do
  target 'MyApp' do
    configuration 'debug' do
    end 

    configuration 'release' do
    end 
  end
end

I personally, don't consider this more scarier than the one liner. Moreover, I think the sentence is pretty clear in the context of the automatically generated Podfile.

pod 'Lookback', :configurations => ['Test']

I'm making the assumption that most of my users have created an Xcode project from one of the templates, and stuck mostly with that for the lifetime of their project, and thus have only one target

This is the problem that I wan't to avoid, this assumption is not clear for non experienced developers. And thus makes the jump much more confusing than what it should be.

@alloy
Copy link
Member

alloy commented Dec 18, 2013

@irrationalfab I’m sorry, but I really disagree with you on this. We should support the one liner and the implementation details should never influence the DSL.

@alloy
Copy link
Member

alloy commented Dec 18, 2013

That is not to say that I don’t think your suggestion should work as well, but definitely not as the only way to do it.

@ghost ghost assigned alloy Dec 19, 2013
@fabiopelosin fabiopelosin changed the title Whitelist pods for specific build configs Add support for the specification of dependencies by configuration Mar 27, 2014
@fabiopelosin fabiopelosin mentioned this pull request Mar 31, 2014
14 tasks
@fabiopelosin
Copy link
Member

Closing in favour of #154.

Re-reading this thread and the related discussion I realise that I didn't handle it in the most optima way. I mixed different changes (thanks @kylef for pointing out the way) and jeopardised the situation. This patch should have landed in CocoaPods a long time ago. I apologies to the users waiting for it and especially to @nevyn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants