Ability to load commands defined in separate files using the standard DSL

[markrebec]

Allows defining commands in standalone ruby files and loading them into the program, which is done via instance_eval in this first attempt at an implementation. The command files can use the standard DSL directly, without needing to worry about the parent program or reference the Commander::Runner singleton instance.

I don't love the instance_eval implementation, but it was the easiest way to get it prototyped without a ton of refactoring, and I believe is still a step in the right direction with this feature.

A few reasons this is useful:

• The couple times I've used commander in projects, I've found myself wanting to organize my commands the same way you might organize rake tasks. Defining these in separate files allows you to create a more navigable structure and encourages treating these resources separately from lib/ (which is admittedly an opinionated decision), and can make it easier to track down what you're looking for when a commander project grows in size and contains many commands.
• We've been building a "toolbelt" for our engineers, to assist with common tasks like provisioning, deploying, environment setup, etc. and we'd like to include/exclude some variations of commands based on various criteria (like which project directory you're currently in). Wrapping load_commands in conditionals allows us to check that criteria and load a different subset of command files at runtime rather than including them all by defining them inline or loading stuff with require.
• This also allows you to include commands from multiple sources. For example, again in our toolbelt, we're letting users of the toolbelt client (our commander project) add custom commands for all their own little one-offs and scattered scripts by placing them into a configured ~/.toolbelt/commands path.
• These files don't have to follow the hello_world.rb extension/pattern. Since the load_command method just takes a path and reads the file, any naming pattern can be used. As the DSL evolves, commander could move more towards the pattern rake uses and define a command file format if desired (maybe something like hello_world.cmd).

[markrebec]

I don't want to tweak that RuboCop config for class length that I triggered without discussing it first.

I'm also unsure why some of the travis builds seem to be failing when trying to bundle install the gem...

[ggilder]

Hmm, I can definitely see your use case but the implementation definitely feels weird... providing a mechanism to eval random files with no protections around it definitely scares me. Also it feels like we'd kind of be reinventing built-in require functionality.

I haven't tried this, but I think it might be possible to achieve the same desired effect by simply wrapping the commands in the separate file in a Commander.configure block, so something like:

# my/commands/hello.rb

Commander.configure do
  command :hello do
    # etc.
  end
end

And then simply using require to load the file.

[markrebec]

Totally understand the hesitation, I'm also a little wary about the file read+eval.

I actually kind of like the Commander.configure block, though I'm curious whether that would work in all situations (i.e. modules)? My guess is that it should just work thanks to the singleton Runner.instance.

I'm going to find a moment to take a look at what Rake does for loading my.task files, just as a reference, and see if anything else maybe jumps to mind as a solution that doesn't involve the instance_eval bit.

Thanks for the feedback! I'll see if I can polish the implementation a bit and come back with something a bit more solid.

If you prefer to do some housekeeping on the repo, feel free to close this PR and I can always open another one.

[markrebec]

I managed to come up with what I think is a somewhat cleaner solution. I've introduced a Commander::Loader singleton class that can be used to load commands using load rather than instance_eval.

The way it works is by surrounding the calls to load with some metaprogramming that temporarily defines the command method on the top-level Object class to allow the same simple DSL usage by proxying the call to Command::Runner.instance, then undefines the method to clean up after ourselves once the command(s) have been loaded.

An alternate approach might be to implement a usage pattern similar to Commander::Import, where at the bottom of lib/commander/loader.rb we statically/permanently define Object.command method and remove the Commander::Loader#with_command_context method completely. Then we could remove the default require 'commander/loader' line from /lib/commander.rb and require users who want to utilize loading commands in files add the require 'commander/loader' line themselves.

I'm hoping this is a bit closer to a more realistic implementation, and would love to hear your thoughts on the current vs. alternate implementation mentioned above.

I'm going to look into the CI failures and see if I can figure out what's up there.

[ggilder]

Hmm, to me that still seems like an awful lot of hoops to jump through to avoid a single Commander.configure block. Can you explain to me the advantages you see over a simpler, less invasive require + Commander.configure approach?

[markrebec]

There are three primary reasons in my head, all of which are admittedly somewhat opinionated.

1. I admit I haven't yet given your suggestion a shot to fully test whether it works in all cases (i.e. the module implementation). As we both mentioned above, the singleton runner instance should let this just work, but this alternate solution was an itch I really wanted to scratch. I'll test this approach more fully as it may prove to be the best approach.
2. I personally really like the simplicity of being able to use the command DSL transparently without the Commander.configure block. Especially if you've got a project with multiple contributors and a growing number of command definitions, I feel the simpler the template for these files the better.
3. I might have a few crossed wires and may be accounting for cases that don't (yet?) exist. I've also started playing with the idea of nested "subcommands" in a separate branch. Basically storing multiple runner instances and swapping the singleton instance in/out based on context. This allows for my_cli command subcommand --subcommand-flag where command would actually be an instance of another "program" rather than "command," and subcommand would be a command within that program context. I have no idea whether this is a feature you'd be interested in, if it's on the roadmap already, or if it has been considered and determined unnecessary.

[markrebec]

I'm still failing rubocop due to the class length rule, but the tweak to .travis.yml in my last commit may be worth noting even if this feature/PR ends up being rejected.

It seems like the version of bundler being used in the CI environment has a bug that's been fixed in later versions, which is why my builds were failing to run. The change to the Travis config just ensures bundler is updated before attempting to bundle install. More info here: https://github.com/rubygems/rubygems/issues/1220

[JulienBreux]

Amazing!

[ggilder]

@markrebec thanks for the Travis fix, I've cherry-picked your commit onto master so you can rebase your branch.

Regarding the main features in this PR, I've been trying to shift commander in the direction of not polluting the global namespace as much as possible, so I'm definitely wary of changes that expose new global methods. See https://github.com/tj/commander/pull/70 for some context. Especially in the context of larger projects, which seems to be what you're looking to address, I think namespacing is essential.

[markrebec]

@ggilder hah, great timing :smile: I was just thinking about shooting another note your way now that the holidays are over and I'm going to be digging back into our project where commander is being used again soon.

I totally understand where you're coming from and why you're a bit wary of this feature. I'm starting to lean towards maybe just releasing an optional/supplemental gem, commander-loader or something along those lines, that provides this functionality. Then, depending on where I end up going with the concept of "subcommands" (if anywhere at all) and how it fits into your vision for the core project, maybe I'll follow the same path there rather than folding it into the commander core.

Any thoughts on a supplemental commander-loader gem?

[ggilder]

Sure, a supplemental gem would be a fine way to add the functionality. I guess I would just say that, having observed the general trend towards reducing usage of the global namespace in general in Ruby gems over the past several years, and the reasons motivating that trend, I would strongly encourage finding ways to follow that tide. :wink:

[markrebec]

Gonna close this one out while I'm tidying up my PRs. I've since moved on from the project that prompted this feature, and since it never quite managed to find it's way into master there's no reason to let it linger :)