Add an `init` command to pod for generating a skeleton Podfile based on an .xcodeproj #1106

Merged
merged 15 commits into from Aug 5, 2013

Conversation

Projects
None yet
4 participants
Contributor

ianyh commented Jun 9, 2013

My ruby is pretty rusty and my rspecfu is not strong, so I am more than open to suggestions and improvements.

I also want to expand it to include a list of pods as arguments to add the Podfile as dependencies, but wasn't sure if that should be a part of this pull request or not. I wanted to get a base implementation down.

Thoughts?

Contributor

ianyh commented Jun 9, 2013

Also, this may be a good place to introduce some idiomatic Podfile structures, but I figured that might be a decision best left up to more members of the github community. I'm open to suggestions on things to add.

Coverage Status

Coverage remained the same when pulling 0cc404f on ianyh:pod-init into b1ac8b2 on CocoaPods:master.

Owner

fabiopelosin commented Jun 10, 2013

Some feedback:

  • It looks like the tests are not actually checking that the targets are appropriate generated in the Podfile. This can be easily done either reading the Podfile and checking.
  • Some unnecessary white space is added inside the target blocks.
  • The best practice is to not to add the platform when the Podfile is being integrated (i.e. almost always) so I would remove it from the generated Podfile.
  • I would be nice to run pod install after the generation so the workspace can be integrated in one shot (this depends on #765).

I also want to expand it to include a list of pods as arguments to add the Podfile as dependencies

I think that this solution would clutter too much the command line interface. In the past some users have proposed to store a list of common Pods in ~/.cocoapods. Maybe we store a list for normal targets and for test targets (which we would identify with some simple heuristic like wether they include the Test word).

this may be a good place to introduce some idiomatic Podfile structures

What do you mean exactly by idiomatic Podfile structures?

Once you patch is finished, could you add a note to the changelog?

Nice work 👍
Related #1045.

@fabiopelosin fabiopelosin commented on an outdated diff Jun 10, 2013

lib/cocoapods/command/init.rb
+ an Xcode project file is specified or if there is only a single project
+ file in the current directory, targets will be automatically generated
+ based on targets defined in the project.
+ DESC
+ self.arguments = '[XCODEPROJ]'
+
+ def initialize(argv)
+ @podfile_path = Pathname.pwd + "Podfile"
+ @project_path = argv.shift_argument
+ @project_paths = Pathname.pwd.children.select { |pn| pn.extname == '.xcodeproj' }
+ super
+ end
+
+ def validate!
+ super
+ help! "Existing Podfile found in directory" if File.file? @podfile_path
@fabiopelosin

fabiopelosin Jun 10, 2013

Owner

It would be more appropriate to check config.podfile.nil? as the Podfile can have different names.

@fabiopelosin fabiopelosin and 1 other commented on an outdated diff Jun 10, 2013

lib/cocoapods/command/init.rb
+ based on targets defined in the project.
+ DESC
+ self.arguments = '[XCODEPROJ]'
+
+ def initialize(argv)
+ @podfile_path = Pathname.pwd + "Podfile"
+ @project_path = argv.shift_argument
+ @project_paths = Pathname.pwd.children.select { |pn| pn.extname == '.xcodeproj' }
+ super
+ end
+
+ def validate!
+ super
+ help! "Existing Podfile found in directory" if File.file? @podfile_path
+ unless @project_path
+ help! "No xcode project found, please specify one" unless @project_paths.length > 0
@fabiopelosin

fabiopelosin Jun 10, 2013

Owner

We tend to use raise Informative, "Message" in CocoaPods.

@ianyh

ianyh Jun 10, 2013

Contributor

All of the command validation uses help!. Should they all use raise Informative?

@fabiopelosin fabiopelosin commented on an outdated diff Jun 10, 2013

lib/cocoapods/command/init.rb
+ help! "Multiple xcode projects found, please specify one" unless @project_paths.length == 1
+ @project_path = @project_paths.first
+ end
+ help! "Xcode project at #{@project_path} does not exist" unless File.exist? @project_path
+ @xcode_project = Xcodeproj::Project.new(@project_path)
+ end
+
+ def run
+ @podfile_path.open('w') { |f| f << podfile_template(@xcode_project) }
+ end
+
+ def podfile_template(project)
+ platforms = project.targets.map { |t| t.platform_name }.uniq
+ has_global_platform = platforms.length == 1
+ if has_global_platform
+ podfile = <<-PLATFORM
@fabiopelosin

fabiopelosin Jun 10, 2013

Owner

To preserve readability we tend to require active_support/core_ext/string/strip at the top of the file and then do something like:

          podfile = <<-PLATFORM.strip_heredoc
            platform :#{platforms.first}
            # [...]
          PLATFORM

@fabiopelosin fabiopelosin commented on an outdated diff Jun 10, 2013

lib/cocoapods/command/init.rb
+ super
+ help! "Existing Podfile found in directory" if File.file? @podfile_path
+ unless @project_path
+ help! "No xcode project found, please specify one" unless @project_paths.length > 0
+ help! "Multiple xcode projects found, please specify one" unless @project_paths.length == 1
+ @project_path = @project_paths.first
+ end
+ help! "Xcode project at #{@project_path} does not exist" unless File.exist? @project_path
+ @xcode_project = Xcodeproj::Project.new(@project_path)
+ end
+
+ def run
+ @podfile_path.open('w') { |f| f << podfile_template(@xcode_project) }
+ end
+
+ def podfile_template(project)
@fabiopelosin

fabiopelosin Jun 10, 2013

Owner

It would be nice to add a comment and mark this method as private.

Owner

fabiopelosin commented Jun 10, 2013

P.S: Don't be scared by me being a pull request nazi 🍻. If I'm too pedantic I can take care of homogenizing the style to the CP codebase.

Contributor

ianyh commented Jun 10, 2013

No worries on the comments, it's exactly what I was looking for. I'll try to patch things up later today.

What do you mean exactly by idiomatic Podfile structures?

I was thinking this would be a good opportunity to better define the best practices for structuring a Podfile or something almost like a guide along the lines of the template used by pod spec create.

Owner

fabiopelosin commented Jun 10, 2013

I was thinking this would be a good opportunity to better define the best practices for structuring a Podfile or something almost like a guide along the lines of the template used by pod spec create.

👍 Sounds good. I think that if you have the time you can go ahead and implement the comments that you think would be suitable.

Coverage Status

Coverage remained the same when pulling 8672d7a on ianyh:pod-init into b1ac8b2 on CocoaPods:master.

Contributor

ianyh commented Jun 11, 2013

I still want to add functionality for using default pods when defined.

Coverage Status

Coverage remained the same when pulling 7b50556 on ianyh:pod-init into b1ac8b2 on CocoaPods:master.

Coverage Status

Coverage remained the same when pulling c911f3e on ianyh:pod-init into b1ac8b2 on CocoaPods:master.

Coverage Status

Coverage remained the same when pulling fadf395 on ianyh:pod-init into b1ac8b2 on CocoaPods:master.

@fabiopelosin fabiopelosin and 1 other commented on an outdated diff Jun 13, 2013

lib/cocoapods/command/init.rb
+ based on targets defined in the project.
+ DESC
+ self.arguments = '[XCODEPROJ]'
+
+ def initialize(argv)
+ @podfile_path = Pathname.pwd + "Podfile"
+ @project_path = argv.shift_argument
+ @project_paths = Pathname.pwd.children.select { |pn| pn.extname == '.xcodeproj' }
+ super
+ end
+
+ def validate!
+ super
+ help! "Existing Podfile found in directory" unless config.podfile.nil?
+ unless @project_path
+ help! "No xcode project found, please specify one" unless @project_paths.length > 0
@fabiopelosin

fabiopelosin Jun 13, 2013

Owner

The #help! method shows the help banner of the command and therefore it is suited to be used when the command or the arguments sent to the command line do not validate. Other user errors are better handled raising an Informative. For example in this case it would be of little help to show the banner of the command as it was correctly written by the user, actually I think that it is confusing in this way.

@ianyh

ianyh Jun 13, 2013

Contributor

Okay, that makes sense.

Owner

fabiopelosin commented Jun 13, 2013

Looks good so far to me, the only issue that I have with this implementation is the proliferation of config files in ~/.cocoapods. Did you explore the possibility of adding two dedicated keys to the ~/.cocoapods/config.yaml file? Maybe storing an array for the name of the Pods?

Contributor

ianyh commented Jun 13, 2013

I considered it, but it becomes hard to do more complicated pods or podspec's that take blocks. This also in theory supports defining pre- and post-installs.

Coverage Status

Coverage remained the same when pulling 79deadc on ianyh:pod-init into b1ac8b2 on CocoaPods:master.

Contributor

ianyh commented Jun 27, 2013

@irrationalfab Is there anything else you want me to do for this change?

Owner

fabiopelosin commented Jun 27, 2013

I'm not totally sold on the configuration files, other than that it looks good to me. I'm asking for feedback to the rest of the team. Anyway, I would prefer to merge this after the 0.21.0 release as we currently in the rc (This change is pretty big for the CP internals).

Contributor

ianyh commented Jun 27, 2013

Totally fair on both counts.

Owner

fabiopelosin commented Jun 28, 2013

After discussing with the rest of the team we agreed that the implementation looks good but it would be better to implement #1150 and store the templates in the .cocoapods/templates subfolder before merging this.

Owner

fabiopelosin commented Aug 1, 2013

This patch can be merged after adding support for the .cocoapods/templates file structure. Not sure if the file name of the templates can be improved (currently default.podfile, test.podfile)

Contributor

ianyh commented Aug 5, 2013

Okay, updated to use new directory structure and renamed the Podfile templates to something maybe more reasonable.

Coverage Status

Coverage remained the same when pulling 092e8cb on ianyh:pod-init into 57abeab on CocoaPods:master.

Coverage Status

Coverage remained the same when pulling cb50c74 on ianyh:pod-init into 57abeab on CocoaPods:master.

fabiopelosin merged commit cb50c74 into CocoaPods:master Aug 5, 2013

1 check failed

default The Travis CI build failed
Details
Owner

fabiopelosin commented Aug 5, 2013

@ianyh You've being doing some awesome work lately. Let me know if you would like to be more involved in the CocoaPods scene by joining our awesome plaza (our Campfire room)! 🍻

Owner

orta commented Aug 5, 2013

🍺

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