Closed krissi closed 10 years ago
The second pull requests implements feature request #61
Thank you for taking a look at that feature request!
There are a few initial things I have questions about.
Commander::Mixin
? Referring to mixin
seems a little generic and also refers to the method of including the module rather than what the module does. Maybe Commander::Methods
?block.rb
? I don't see it used anywhere.Good point, I renamed the module to Commander::Methods
.
While adding the specs I had to rebuild the other specs and the spec_helper.rb
. The spec_helper.rb
itself included the modules into the global namespace, which made my specs impossible.
I moved the include
statements to the specs. In fact this statements are a dup of Commander::Methods. I changed this in commit 3f8958f.
To 2: The intention of block.rb
is the feature request #61:
Commander.configure do
program :name, 'Foobar'
program :version, '1.0'
# ...
command :foo do
# ...
end
end
This might be added to Commander
-Class itself.
Hey, sorry for the delay getting back to you on this.
I played around with the branch a bit, and noticed a few things:
require "commander"
— it could be moved into the main module.lib/commander/core_ext
that monkey-patch existing classes, which might be considered similarly invasive.Commander::Methods
into the describe
blocks in the specs? I'm not sure why that would be needed...Hi,
we need to include the module, because the spec_helper changed in f4e0c0bd1befa274327f60998c6a90057a8f41bf.
Previously the helper included the modules in the global namespace. This needed to be removed because is was the import.rb
-way, not the methods.rb
-way.
Gotcha, that makes sense now
Hi, just taking another look at this. It looks good and I think we can tackle the other global namespace pollution issues later.
However, I pulled down the branch to test and noticed a few issues with the tests. One is that your tests use newer RSpec syntax (e.g. allow
), so we should bump the version of RSpec required in commander.gemspec
. I would just go to ~> 2.14
.
Another weird issue... when I run the tests on this branch, something weird happens with output paging, and the output gets redirected to less
. This seems to prevent the tests from returning a correct exit code, which could break the Travis build. I'll poke around and see if I can figure out why that's happening, but please take a look and see if you can find the cause as well.
Found the problem, see new spec_helper.rb
. I think it was never a problem, because it is guarded by $stdout.tty?
I have changed the mock to force stdout to be not a tty and let the method return early by itself. I thing this way is cleaner as it does not remove the method globally and the stdout-mock may be easily changed on a per-spec basis when needed.
I reverted to simple mocking
I think this is looking really good. Thanks again for putting in so much work on this!
Before merging it looks like the branch should be rebased on master (Github is showing merge conflicts). Also, I think the commit history can be squashed down to a few atomic commits. Let me know if you want to give that a shot; I can also take care of it if you're not familiar with rebasing and squashing.
I tried my best. As it was the first time I hope I did not crash everything ;)
The squashed commits look great! As far as rebasing on master, I realize now I should have explained that better because it's a little more complex with forks. I've gone ahead and merged the branch manually. Thank you!
Could you please deploy the features to rubygems?
Just released a new version of the gem - 4.2.0. Thanks again!
This Mixin may be included in a specific class to keep the global namespace clean.
Example: