commander-rb / commander

The complete solution for Ruby command-line executables
MIT License
822 stars 74 forks source link

How do I access the set of options that were given to the active command from within code called by that command? #70

Closed ohTHATaaronbrown closed 6 years ago

ohTHATaaronbrown commented 6 years ago

In v4.4.4 and before, i could access Commander::Runner.instance&.active_command&.proxy_option_struct from within my library code called by a command, and get access to the options that were handed to the actively running command.

v4.4.5 seems to have broken that functionality. Is there an alternative way that I should use instead?

ggilder commented 6 years ago

Hmm, the only difference in that version is https://github.com/commander-rb/commander/commit/935e0f679135d315c40cd16b8bf430cb80dd429f by @doriantaylor , but that doesn't modify the proxy_option_struct method so I'm not sure why that would have changed. Can you post minimal sample code that works on v4.4.4 and fails on v4.4.5?

ohTHATaaronbrown commented 6 years ago

Hmmm... I've negated my own statement when I went back and tried to make a simple reproduction.

I can't get Commander::Runner.instance&.active_command&.proxy_option_struct to return anything at all, as far back as i go with commander versions. Yet I know it worked at some point, because I used that as the basis for this command that is now broken.

So, I'm instead going to punt here, and ask:

If Commander::Runner.instance&.active_command&.proxy_option_struct does not contain the parameters that were handed to the currently executing command, what does? How do I get access to the set of options that were given to the currently executing command from inside code called by that command? Other than storing all of the options manually for myself in each and every command that I write?

doriantaylor commented 6 years ago

So, this was a little bit hairy from what I recall. This whole construct is brimming with statefulness and most of it probably isn't necessary. Indeed I call proxy_options.clear because if I didn't, they would just keep accumulating as the command gets re-run. proxy_option_struct is a method that depends on the contents of proxy_options which itself just wraps the instance variable@proxy_options. If proxy_options is empty (which it has to be if you want to re-run the command), then proxy_option_struct will return an empty object.

A band-aid solution would be to memoize proxy_option_struct so it will reliably return a result, but my inclination is to redesign this entire interaction, possibly with a more explicit naming scheme (e.g. last_run_options, and a last_run_args which currently does not exist).

ohTHATaaronbrown commented 6 years ago

I think I'd suggest active_run_options and active_run_args, to keep with the naming you have with active_command, but I like that idea.

I don't care how it is that I get access to the currently active command's args and options, I just want to be able to do it somehow. :)

doriantaylor commented 6 years ago

cool; my involvement in this project is literally that one patch so I could re-run commands in a shell. @ggilder is the authority here.

ohTHATaaronbrown commented 6 years ago

@doriantaylor I was looking at your addition there, and wondering why you couldn't call proxy_options.clear after the proc/object call below? Wouldn't that accomplish both things? It'd leave proxy_option_struct intact for the current run, but clear it before the next.

Or am I reading that wrong?

doriantaylor commented 6 years ago

wouldn't make a difference

ggilder commented 6 years ago

@doriantaylor @ohTHATaaronbrown I have a slightly different approach — see https://github.com/commander-rb/commander/tree/ggilder/empty-proxy-options-before-parse

Let me know if that works for both of your use cases.

The code here is definitely pretty gnarly, I'd certainly be open to a more thoughtful design but I don't have the bandwidth to put one together.

ohTHATaaronbrown commented 6 years ago

@ggilder I like that approach. That feels clean to me, and makes perfect sense: clear out any existing remnants before populating from a parse for a new call.

ohTHATaaronbrown commented 6 years ago

Either of the approaches would work for me, i think.

ggilder commented 6 years ago

@ohTHATaaronbrown Can you test with that version to see if your code works? You should be able to change your Gemfile to reference that branch using

gem 'commander', :git => 'https://github.com/commander-rb/commander.git', :branch => 'ggilder/empty-proxy-options-before-parse'
doriantaylor commented 6 years ago

My only concern with that approach is I don't know if there's a use case where the command instance would be invoked via call without having first been put through run (certainly nothing of mine), however I'm indifferent as long as the tests from 935e0f6 pass.

ggilder commented 6 years ago

Thanks, good point — I think (if I'm reading the code correctly) that everything has to go through run to set up the state. There could be 3rd-party code doing weird things with call directly but I think that's pretty unlikely.

ggilder commented 6 years ago

@doriantaylor I did modify the test you added slightly — let me know if it still seems like the correct expectation.

ohTHATaaronbrown commented 6 years ago

@ggilder Hmm... It's still not behaving as I'd expect.

https://gist.github.com/ohTHATaaronbrown/68607e1677fd6e3ce722ec3c0db9a271

Contents of the Gemfile:

source 'https://rubygems.org'

gem 'commander', :git => 'https://github.com/commander-rb/commander.git', :branch => 'ggilder/empty-proxy-options-before-parse'
gem 'logging'

That's a very streamlined version of how I'm using things. What am I doing wrong?

ggilder commented 6 years ago

Hmm, you're running that example using bundle exec ? I see a different error when running that example with that Gemfile using bundle exec ./test set_tenant_list -t installed -l d:

ERROR  Blue : set_tenant_list: Problem setting tenant list to ["installed"]. Error: undefined method `downcase' for nil:NilClass
ERROR  Blue : <NoMethodError> undefined method `downcase' for nil:NilClass
    /usr/local/lib/ruby/gems/2.5.0/bundler/gems/commander-deb469a95820/lib/commander/user_interaction.rb:359:in `method_missing'
    /Users/gabriel/Desktop/test/test.rb:175:in `computer'
    /Users/gabriel/Desktop/test/test.rb:222:in `set_tenant_list'
    /Users/gabriel/Desktop/test/test.rb:258:in `block in <top (required)>'
    /usr/local/lib/ruby/gems/2.5.0/bundler/gems/commander-deb469a95820/lib/commander/runner.rb:400:in `block in global_option_proc'
    /usr/local/Cellar/ruby/2.5.0/lib/ruby/2.5.0/optparse.rb:1584:in `block in parse_in_order'
    /usr/local/Cellar/ruby/2.5.0/lib/ruby/2.5.0/optparse.rb:1538:in `catch'
    /usr/local/Cellar/ruby/2.5.0/lib/ruby/2.5.0/optparse.rb:1538:in `parse_in_order'
    /usr/local/Cellar/ruby/2.5.0/lib/ruby/2.5.0/optparse.rb:1532:in `order!'
    /usr/local/Cellar/ruby/2.5.0/lib/ruby/2.5.0/optparse.rb:1626:in `permute!'
    /usr/local/Cellar/ruby/2.5.0/lib/ruby/2.5.0/optparse.rb:1648:in `parse!'
    /usr/local/lib/ruby/gems/2.5.0/bundler/gems/commander-deb469a95820/lib/commander/runner.rb:381:in `parse_global_options'
    /usr/local/lib/ruby/gems/2.5.0/bundler/gems/commander-deb469a95820/lib/commander/runner.rb:65:in `run!'
    /usr/local/lib/ruby/gems/2.5.0/bundler/gems/commander-deb469a95820/lib/commander/delegates.rb:15:in `run!'
    /usr/local/lib/ruby/gems/2.5.0/bundler/gems/commander-deb469a95820/lib/commander/import.rb:5:in `block in <top (required)>'
 INFO  Object : Tenant list provided to set_tenant_list: ["installed"]
 WARN  Blue::ParameterItemStack : No tenant in list. Skipping execution of code block.
 INFO  Object : Tenants retrieved from Blue.options.tenants: []
DEBUG  Blue : set_tenant_list.proxy_option_struct: <Commander::Command::Options tenants=["installed"], loglevel="d">
 INFO  Blue : Tenants retrieved from active_command.proxy_option_struct.tenants: ["installed"]
 INFO  Object : Tenants retrieved from Blue.active_command_tenants: ["installed"]
ohTHATaaronbrown commented 6 years ago

Ah, I hadn't run it with bundle exec, no. However, when I do, it now behaves as I'd expect it to:

Aaron@ABLaptop MINGW64 /c/work/test_proxy_option_struct
$ bundle install
Using bundler 1.16.1
Using highline 1.7.10
Using commander 4.4.5 from https://github.com/commander-rb/commander.git (at ggilder/empty-proxy-options-before-parse@deb469a)
Using little-plugger 1.1.4
Using multi_json 1.13.1
Using logging 2.2.2
Bundle complete! 2 Gemfile dependencies, 6 gems now installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.

Aaron@ABLaptop MINGW64 /c/work/test_proxy_option_struct
$ bundle exec ruby ./test set_tenant_list -t installed -l d
 INFO  Object : Tenant list provided to set_tenant_list: ["installed"]
DEBUG  Blue::ParameterItemStack : Pushed current tenant tenantA
 INFO  Object : Current in-scope tenant: tenantA
DEBUG  Blue::ParameterItemStack : Popped current tenant to nil
DEBUG  Blue::ParameterItemStack : Pushed current tenant tenantC
 INFO  Object : Current in-scope tenant: tenantC
DEBUG  Blue::ParameterItemStack : Popped current tenant to nil
 INFO  Object : Tenants retrieved from Blue.options.tenants: ["tenantA", "tenantC"]
DEBUG  Blue : set_tenant_list.proxy_option_struct: <Commander::Command::Options tenants=["installed"], loglevel="d">
 INFO  Blue : Tenants retrieved from active_command.proxy_option_struct.tenants: ["installed"]
 INFO  Object : Tenants retrieved from Blue.active_command_tenants: ["installed"]

I am, by the way, running this in ruby 2.3.3 on Windows 10 x64, in case that makes any difference.

Once I made sure to run it through bundler, I'll definitely give my thumbs-up to your solution.

Sorry for the cockup on my part there.

ohTHATaaronbrown commented 6 years ago

Think I could get you to release this to rubygems?

ohTHATaaronbrown commented 6 years ago

Ah, I see the problem you encountered. I use ENV['COMPUTERNAME'] in the code, which is always set on the windows machines where I'm running this code, but probably not anywhere else. It was the downcasing of that that was blowing up.

ggilder commented 6 years ago

Cool, that totally makes sense. Glad it's working as expected now. I'll go ahead and merge #72 and update you when it's released.

ggilder commented 6 years ago

I've released 4.4.6 with this fix. Thanks for identifying the issue, and thanks @doriantaylor for your help diagnosing!

ohTHATaaronbrown commented 6 years ago

@ggilder @doriantaylor thanks for jumping on this so quickly and helping me!