Implement --web flag for pod search (see issue #1679). #1682

Merged
merged 1 commit into from Dec 14, 2013

Conversation

Projects
None yet
2 participants
@floere
Owner

floere commented Dec 14, 2013

Hey guys,

This implements #1679. I frankly was unsure how to test this sufficiently – perhaps mock a system call. Let me know what you usually do in the project.

Cheers!

@floere

This comment has been minimized.

Show comment Hide comment
@floere

floere Dec 14, 2013

Owner

Heh, I am having a lot of fun using the command line web search :D

Owner

floere commented Dec 14, 2013

Heh, I am having a lot of fun using the command line web search :D

lib/cocoapods/command/search.rb
@@ -36,14 +38,29 @@ def validate!
end
def run
- sets = SourcesManager.search_by_name(@query.strip, @full_text_search)
+ @web ? web_search : local_search

This comment has been minimized.

Show comment Hide comment
@fabiopelosin

fabiopelosin Dec 14, 2013

Owner

Continuing our discussion, in this case I prefer the tradition if indentation because it makes the control flow explicit. In my style use this format only for assignments.

@fabiopelosin

fabiopelosin Dec 14, 2013

Owner

Continuing our discussion, in this case I prefer the tradition if indentation because it makes the control flow explicit. In my style use this format only for assignments.

This comment has been minimized.

Show comment Hide comment
@floere

floere Dec 14, 2013

Owner

So if ... else ... end?

@floere

floere Dec 14, 2013

Owner

So if ... else ... end?

This comment has been minimized.

Show comment Hide comment
@fabiopelosin

fabiopelosin Dec 14, 2013

Owner

Yep that is my way. For me an important rule for clarity is:

logic branching = indentation
@fabiopelosin

fabiopelosin Dec 14, 2013

Owner

Yep that is my way. For me an important rule for clarity is:

logic branching = indentation
lib/cocoapods/command/search.rb
if @supported_on_ios
sets.reject!{ |set| !set.specification.available_platforms.map(&:name).include?(:ios) }
end
if @supported_on_osx
sets.reject!{ |set| !set.specification.available_platforms.map(&:name).include?(:osx) }
end
-
+

This comment has been minimized.

Show comment Hide comment
@fabiopelosin

fabiopelosin Dec 14, 2013

Owner

😄

This comment has been minimized.

Show comment Hide comment
@floere

floere Dec 14, 2013

Owner

It's very important! ;)

@floere

floere Dec 14, 2013

Owner

It's very important! ;)

@fabiopelosin

This comment has been minimized.

Show comment Hide comment
@fabiopelosin

fabiopelosin Dec 14, 2013

Owner

Looks good to me, could you add tests for the new behavior and an entry to the changelog?

Owner

fabiopelosin commented Dec 14, 2013

Looks good to me, could you add tests for the new behavior and an entry to the changelog?

@fabiopelosin

This comment has been minimized.

Show comment Hide comment
@fabiopelosin

fabiopelosin Dec 14, 2013

Owner

Then I would suggest to squash all the commits and cherry pick the result in master.

Owner

fabiopelosin commented Dec 14, 2013

Then I would suggest to squash all the commits and cherry pick the result in master.

@floere

This comment has been minimized.

Show comment Hide comment
@floere

floere Dec 14, 2013

Owner

How do you usually test (mock) system calls? (see the pull request text)

Owner

floere commented Dec 14, 2013

How do you usually test (mock) system calls? (see the pull request text)

@fabiopelosin

This comment has been minimized.

Show comment Hide comment
@fabiopelosin

fabiopelosin Dec 14, 2013

Owner

I would either (in order of preference):

  • Include the excitable mixin and declare the open excitable
  • Factor out the system call to a method (open_in_browser or something like that)
  • or
Kernel.expects( :` ).with('open url')
Owner

fabiopelosin commented Dec 14, 2013

I would either (in order of preference):

  • Include the excitable mixin and declare the open excitable
  • Factor out the system call to a method (open_in_browser or something like that)
  • or
Kernel.expects( :` ).with('open url')

floere added a commit that referenced this pull request Dec 14, 2013

@floere

This comment has been minimized.

Show comment Hide comment
@floere

floere Dec 14, 2013

Owner

Alright, now including Executable and nice specs. Cheers!

Owner

floere commented Dec 14, 2013

Alright, now including Executable and nice specs. Cheers!

@floere

This comment has been minimized.

Show comment Hide comment
@floere

floere Dec 14, 2013

Owner

CHANGELOG coming up.

Owner

floere commented Dec 14, 2013

CHANGELOG coming up.

floere added a commit that referenced this pull request Dec 14, 2013

@floere

This comment has been minimized.

Show comment Hide comment
@floere

floere Dec 14, 2013

Owner

Ok, good to go from my end.

Owner

floere commented Dec 14, 2013

Ok, good to go from my end.

@floere floere merged commit f9fa8db into master Dec 14, 2013

1 check was pending

default The Travis CI build is in progress
Details
@floere

This comment has been minimized.

Show comment Hide comment
@floere

floere Dec 14, 2013

Owner

Thanks for the guidance, @irrationalfab! 👍

Owner

floere commented Dec 14, 2013

Thanks for the guidance, @irrationalfab! 👍

@floere floere deleted the search-web branch Dec 14, 2013

@fabiopelosin

This comment has been minimized.

Show comment Hide comment
@fabiopelosin

fabiopelosin Dec 14, 2013

Owner

Thank you for the awesome patch!

Owner

fabiopelosin commented Dec 14, 2013

Thank you for the awesome patch!

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