Making `pod plugins` an abstract command with search as default subcommand #13

AliSoftware · 2014-04-29T21:09:35Z

pod plugins is now an abstract command (fixes Refactor Plugins as an abstract command #11)
~~search is now the default subcommand (fixes Make search the default subcommand #12) and accepts an empty query (making it list all the plugins)~~ [EDIT] See below, now the new list subcommand is the default one
Some refactoring has been done, especially splitting classes and bacon specs in separate files (fixes Refactor plugins.rb into one command class per file #10)
Updated specs accodingly

…th `search` as the default command (#12)

segiddins · 2014-04-29T21:33:28Z

spec/command/plugins/create_spec.rb

+
+    it 'should download the default template repository' do
+      @command = create_command(argv('cocoapods-banana'))
+      # @command = Command::Plugins::Create.new(argv("cocoapods-banana"))


Let's not leave commented-out code

😲 where does this line come from ?!
Agreed.

BTW feel free to add some more specs if you see some use cases I may have forgotten.

BTW, as a ruby newbie, I was wondering: what are the naming conventions regarding test labels?

should we always start the test name with should … to have some consistency, namely should I refactor specs always using it 'should XXX' do?

or is it not such a big deal and is it ok (naming-convention-wise) to keep labels such as it 'prints out all plugins when no query passed' do or it 'registers itself' do?

I suspect that every test label should start with should to make the spec more english-like and readable, but the initial cocoapods-plugins specs by @dbgrandi didn't respect this convention so I'm wondering 😉

@AliSoftware I think that the quoted part of the test should make and english sentence when read inline with the it method and should be as short a possible. For example I prefer:

it 'prints something'

to

it 'should print something'

as the should is just white-noise as it doesn't convey any additional information.

Ok, I guess there is no real convention after all.
I often saw it 'should ...' but am ok with keeping the label short, as long as we are… yeah you guessed it… consistent 😆 across all CP code 😉

AliSoftware · 2014-04-29T21:57:29Z

New commit available which:

fixes the comment pointed out by @segiddins
gets rid of SpecHelper::PluginCreateCommand#argv and SpecHelper::PluginSearchCommand#argv for simplicity

(see discussion in #11)

* Rollbacking to 'search' subcommand requiring a non-empty QUERY. * Refactoring to mutualize code common to 'list' and 'search' in a PluginsHelper module See discussion in #11 and #12

AliSoftware · 2014-05-01T18:35:07Z

New refactoring available, guys!
(the title of this PR should be modified accordingly, btw… I hesitated about starting a new PR but as I pushed all this on my master branch anyway…)

list subcommand added, which is now the default subcommand
the QUERY parameter of the search subcommand is back to mandatory again
Some code refactoring to mutualize the common code used by Pod::Command::Plugins::List and Pod::Command::Plugins::Search in a Pod::PluginsHelper module

This should match the decisions we took in discussions #11 and #12

Note: code review welcome, especially about:

my various usages of require vs. extend: never really sure when to use which
my usage of require File.expand_path '...', File.dirname(__FILE__) vs require 'pod/xxx': when should we use one instead of the other? I was told using require File.expand_path ... is for files local to the project, but seems like the require 'pod/command/plugins/create' syntax was used in plugins.rb and in other places in the original code, so I'm a bit lost on this.
Also saw some require File.expand_path '...', __FILE__ in some code, without the File.dirname(__FILE__)… not sure why the latter would work as File::expand_path seems to require a directory path?!

AliSoftware · 2014-05-01T18:52:35Z

(the title of this PR should be modified accordingly, btw… I hesitated about starting a new PR but as I pushed all this on my master branch anyway…)

PS: @dbgrandi @irrationalfab If you guys feel like the code is ok, maybe we could merge this by now, and close #10, #11 & #12 ? (and even maybe publish 0.1.1)?
Then use new issues to continue the discussion about possible future evolutions?

dbgrandi · 2014-05-01T21:49:55Z

spec/spec_helper.rb

-    end
+  # Add this as an extension into the Search and List specs to help stub the plugins.json request
+  module PluginsStubs
+    require File.expand_path '../lib/pod/plugins_helper', File.dirname(__FILE__)


This should not be needed as plugins_helper should already be loaded from the require 'cocoapods_plugin' above.

This should not be needed as plugins_helper should already be loaded from the require 'cocoapods_plugin' above.

You're right. Will fix it right away.

FYI, I added this line in an attempt to fix an issue I had when previously referencing PluginsHelper::PLUGINS_URL two lines below instead of Pod::PluginsHelper::PLUGINS_URL.

Ruby was then unable to resolve the PluginsHelper constant, trying to resolve it as SpecHelper::PluginsStubs::PluginsHelper::PLUGINS_URL instead of the expected value. By the time I realized I just had to use Pod::PluginsHelper::PLUGINS_URL instead of just PluginsHelper::PLUGINS_URL to make it work, I forgot to remove this require statement that was an attempt to fix this constant resolution issue I had 😉

dbgrandi · 2014-05-01T21:57:30Z

@AliSoftware Ace job! This PR is starting to encompass multiple issues, so 👍 on getting this merged in.

Only feedback I have here is that I'd move the plugins_helper.rb and plugins_helper_spec.rb down one level (to inside command).

Using the standard require "pod/xxx" will search through the ruby load path. This is nice because it allows for alternate implementations of libraries. This alternative implementation could be things like a fix for a bug or a custom mock object for easier third party testing.

Use the more specific require File.expand_path("../../../spec_helper", __FILE__) when it's imperative that you load a specific file, or a file that is not in the load path. In the case of specs, it also makes it easier to run an individual spec with just a bundle exec ruby spec/command/plugins/search_spec.rb.

File.expand_path takes an optional second argument that defaults to the current working directory.

AliSoftware · 2014-05-01T22:00:26Z

Only feedback I have here is that I'd move the plugins_helper.rb and plugins_helper_spec.rb down one level (to inside command).

Actually I hesitated about where I should put those files in the first place. Do you suggest that, in addition to moving them in the command folder, we also need to move the modules in the Pod::Command namespace (adding module Command after module Pod at the beginning of those files) too?

AliSoftware · 2014-05-01T22:05:54Z

File.expand_path takes an optional second argument that defaults to the current working directory.

That's what I understood from the rdocs, so why do we sometimes write File.expand_path("../../../spec_helper", __FILE__) and not File.expand_path("../../../spec_helper", File.dirname(__FILE__)) ?!

Is the File::expand_path method quite lenient and applies File::dirname to this second parameter if it realizes it is actually a file (like when we pass __FILE__ directly) instead of a directory? (If such, I wasn't able to see any hint about this in the rdocs)

AliSoftware · 2014-05-01T22:10:44Z

it also makes it easier to run an individual spec with just a bundle exec ruby spec/command/plugins/search_spec.rb.

Ok that's what I guessed and why I wrote require 'pod/xxx' everywhere in the files under lib/** but require File.expand_path(..., File.dirname(__FILE__)) everywhere under spec/** so I could easily bundle exec bacon xxx_spec.rb 😃

fabiopelosin · 2014-05-02T00:09:31Z

lib/pod/command/plugins/list.rb

+        DESC
+
+        def self.options
+          super.reject { |option, _| option == '--silent' }


It might make sense to port this logic to CLAide /c @alloy

Not sure, it’s quite simple to grasp if you know Ruby. What would you propose?

@Allow I agree that this is pretty annoying to have to .concat(super).reject { } every time we create a subcommand (to get the common options like --verbose and --help and all with Array#concat(super)… but except some like --silent in some cases with Array#reject)

Maybe we can have helper methods in Pod::Command if not in CLAide::Command, like self.common_options (even with some parameter to handle multiple cases like commands designated to only output stuff and do nothing else, for which options like --silent wouldn't have much sense), so that we could Array#concat with this instead of super?

At least I'm sure it's worth thinking about sthg to avoid this concat+reject pattern that we observe in a lot of pod subcommands.

I think it’s definitely worth it to create a CLAide ticket about it to remind us to reflect on this in the future.

@alloy @irrationalfab Ticket opened!

fabiopelosin · 2014-05-02T00:12:57Z

Looks good to me 👍 for the merge!

fabiopelosin · 2014-05-02T00:16:17Z

Some lines are too long but that is another issue: #17.

fabiopelosin · 2014-05-02T00:20:11Z

@AliSoftware ⭐ 🌟 🎆 As your help with the issues has been invaluable I granted push access to you... just use this power wisely by asking via a pull request if you are unsure about a change.

To celebrate I will let to you the pleasure to push the green button. Great work!

/c @CocoaPods/owners

dbgrandi · 2014-05-02T00:21:01Z

Do you suggest that, in addition to moving them in the command folder, we also need to move the modules in the Pod::Command namespace (adding module Command after module Pod at the beginning of those files) too?

Yes

That's what I understood from the rdocs, so why do we sometimes write File.expand_path("../../../spec_helper", FILE) and not File.expand_path("../../../spec_helper", File.dirname(FILE)) ?!

To save space/time. These are equivalent.

require File.expand_path('../spec_helper.rb', __FILE__)
require File.expand_path('spec_helper.rb', File.dirname(__FILE__))

If you do a search across the main cocoapods project, you'll find that any case where a raw __FILE__ is used, the path string starts with a ../.

AliSoftware · 2014-05-02T00:33:55Z

To celebrate I will let to you the pleasure to push the green button. Great work!

Yay! \o/

Making `pod plugins` an abstract command with list as default subcommand

dbgrandi · 2014-05-02T00:37:10Z

🍻

@dbgrandi

(as suggested by @dbgrandi in #13)

orta · 2014-05-02T07:04:25Z

🍻

alloy · 2014-05-02T10:41:01Z

As your help with the issues has been invaluable I granted push access to you...

Fully agreed!

@AliSoftware 🍻

AliSoftware · 2014-05-02T10:43:58Z

Thx guys 🍻

Refactoring (#10) + Making pod plugins an abstract command (#11) wi…

e7f13c0

…th `search` as the default command (#12)

segiddins reviewed Apr 29, 2014
View reviewed changes

Small fixes in spec (removed argv function, removed commented-out line)

67a03da

AliSoftware mentioned this pull request Apr 29, 2014

Adding 'search' subcommand #9

Merged

AliSoftware added 4 commits April 30, 2014 01:24

Merge branch 'CocoaPods/master' into 'AliSoftware/master'

9728c69

Improved help message a bit

6e81455

(see discussion in #11)

Added brackets around QUERY argument to let it show it's optional now.

1d9eaa0

* Adding the 'list' subcommand, making it the default

93fbea4

* Rollbacking to 'search' subcommand requiring a non-empty QUERY. * Refactoring to mutualize code common to 'list' and 'search' in a PluginsHelper module See discussion in #11 and #12

This was referenced May 1, 2014

Make search the default subcommand #12

Closed

Refactor Plugins as an abstract command #11

Closed

dbgrandi reviewed May 1, 2014
View reviewed changes

Removed useless require statement

4fc74ca

AliSoftware added 2 commits May 2, 2014 00:16

small rdoc fixes

55fe55f

I'm a perfectionist.

e1ddea1

fabiopelosin reviewed May 2, 2014
View reviewed changes

AliSoftware added a commit that referenced this pull request May 2, 2014

Merge pull request #13 from AliSoftware/master

fad017e

Making `pod plugins` an abstract command with list as default subcommand

AliSoftware merged commit fad017e into CocoaPods:master May 2, 2014

AliSoftware added a commit that referenced this pull request May 2, 2014

Moved PluginsHelper to the Pod::Command namespace

701080f

(as suggested by @dbgrandi in #13)

AliSoftware mentioned this pull request May 2, 2014

Find a better way to deal with options + inheritance CocoaPods/CLAide#15

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making `pod plugins` an abstract command with search as default subcommand #13

Making `pod plugins` an abstract command with search as default subcommand #13

AliSoftware commented Apr 29, 2014

segiddins Apr 29, 2014

AliSoftware Apr 29, 2014

AliSoftware Apr 29, 2014

fabiopelosin Apr 29, 2014

AliSoftware Apr 29, 2014

fabiopelosin Apr 29, 2014

AliSoftware commented Apr 29, 2014

AliSoftware commented May 1, 2014

AliSoftware commented May 1, 2014

dbgrandi May 1, 2014

AliSoftware May 1, 2014

dbgrandi commented May 1, 2014

AliSoftware commented May 1, 2014

AliSoftware commented May 1, 2014

AliSoftware commented May 1, 2014

fabiopelosin May 2, 2014

alloy May 2, 2014

AliSoftware May 2, 2014

alloy May 2, 2014

AliSoftware May 2, 2014

fabiopelosin commented May 2, 2014

fabiopelosin commented May 2, 2014

fabiopelosin commented May 2, 2014

dbgrandi commented May 2, 2014

AliSoftware commented May 2, 2014

dbgrandi commented May 2, 2014

orta commented May 2, 2014

alloy commented May 2, 2014

AliSoftware commented May 2, 2014

Making pod plugins an abstract command with search as default subcommand #13

Making pod plugins an abstract command with search as default subcommand #13

Conversation

AliSoftware commented Apr 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware commented Apr 29, 2014

AliSoftware commented May 1, 2014

AliSoftware commented May 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbgrandi commented May 1, 2014

AliSoftware commented May 1, 2014

AliSoftware commented May 1, 2014

AliSoftware commented May 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiopelosin commented May 2, 2014

fabiopelosin commented May 2, 2014

fabiopelosin commented May 2, 2014

dbgrandi commented May 2, 2014

AliSoftware commented May 2, 2014

dbgrandi commented May 2, 2014

orta commented May 2, 2014

alloy commented May 2, 2014

AliSoftware commented May 2, 2014

Making `pod plugins` an abstract command with search as default subcommand #13

Making `pod plugins` an abstract command with search as default subcommand #13