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

Allow download strategies to preprocess download options #57

Merged
merged 8 commits into from
Jun 10, 2016

Conversation

champo
Copy link
Contributor

@champo champo commented May 20, 2016

For now, this is useful only for git when downloading a specific branch.

Related to CocoaPods/CocoaPods#5376.

TODO

  • Add test coverage
  • Add to README
  • Add to changelog

For now, this is useful only for git when downloading a specific branch.
@champo
Copy link
Contributor Author

champo commented May 20, 2016

Trying to add test coverage I realized that Executable.execute_command is not available in this gem. And my ruby-fu is not strong enough to use API.execute_command from this gem.

command = ['ls-remote',
options[:git],
options[:branch]]
output = Executable.execute_command('git', command)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just use git command or git! command

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those are instance methods, so they're not available here. it's a class method to avoid instancing downloaders just to preprocess options.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah silly me. we'll figure out how to make that work :) In the meantime, you can do something hacky that'll let you start writing tests / playing around with it if you want

Copy link
Member

@segiddins segiddins May 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diff --git a/lib/cocoapods-downloader/api.rb b/lib/cocoapods-downloader/api.rb
index 9375f10..2e6f079 100644
--- a/lib/cocoapods-downloader/api.rb
+++ b/lib/cocoapods-downloader/api.rb
@@ -4,6 +4,8 @@ module Pod
     # the UI of other gems.
     #
     module API
+      module_function
+
       # Executes
       # @return [String] the output of the command.
       #

and then use API.execute_command

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively,

diff --git a/lib/cocoapods-downloader/api_exposable.rb b/lib/cocoapods-downloader/api_exposable.rb
index 0ed5f3a..efcf236 100644
--- a/lib/cocoapods-downloader/api_exposable.rb
+++ b/lib/cocoapods-downloader/api_exposable.rb
@@ -12,6 +12,7 @@ module Pod
           raise 'Only a module *or* is required, not both.'
         end
         include mod
+        extend mod
       end

       alias override_api expose_api

so overriding still works

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I went with the second patch.

"`#{options.inspect}`."
end

klass = downloader_class_by_key[strategy]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe extract this into downloader_class_from_options to avoid duplication?

@champo
Copy link
Contributor Author

champo commented May 30, 2016

@segiddins I think this is ready, can you give it a look? thanks!

* Allow download strategies to preprocess download options. This is used by
`git` strategy to resolve branches into commits directly.
[Juan Civile](https://github.com/champo)
[Cocoapods#5386](https://github.com/CocoaPods/CocoaPods/pull/5386)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CocoaPods

@champo
Copy link
Contributor Author

champo commented May 31, 2016

@segiddins done!

@segiddins
Copy link
Member

👍🏻

@segiddins segiddins merged commit 0803d22 into CocoaPods:master Jun 10, 2016
@champo champo deleted the preprocessor branch June 10, 2016 12:41
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.

3 participants