stecman / symfony-console-completion

Automatic tab-key completion for Symfony console application options, arguments and parameters
MIT License
421 stars 26 forks source link

Allow commands to complete their option/argument values #19

Closed aik099 closed 9 years ago

aik099 commented 9 years ago

Problems I'm facing right now:

  1. definitions of completion handlers for all commands happens in central place and not inside command
    • when changing command argument list it's easy to miss auto-complete code (which is placed in other class)
    • when command name changes the it's hard to find it's auto-complete
  2. impossible to define catch all command aware completion handler, that will:
    • be located inside a command class
    • respond to all option/argument value auto-completion requests

Current extension point runCompletion method sets command after all handlers are set and doesn't inform them.

Any ideas? I can provide the PR after we discuss possible implementation ways.

aik099 commented 9 years ago

Maybe CompletionInterface::run method can accept following:

Then if support is added for null as $targetName and $type during completion handler registration to act as match any, then it should do the trick.

Maybe also create interface to mark commands as knowing how to auto-complete themselves.

aik099 commented 9 years ago

@stecman , what are your thoughts?

stecman commented 9 years ago

@aik099 - thanks for the PR; it's clearer what you're suggesting now. I like the idea, I'm just not sure about the best way to implement it...In your PR, passing context to CompletionInterface in order to call methods on another interface fits with the current configuration paradigm for the library, but it seems a bit superfluous/complex.

What if the changes to CompletionInterface were dropped, and CompletionHandler became responsible for delegating completion to commands implementing CompletionAwareInterface? So something like:

class CompletionHandler
{
    ...

    protected function completeOption(InputOption $option)
    {
        if ($helper = $this->getCompletionHelper($option->getName(), Completion::TYPE_OPTION)) {
            return $helper->run();
        }

        if ($this->command instanceof CompletionAwareInterface) {
            return $this->command->completeForOption($option->getName());
        }
    }

    ...
}

This implementation would also mean that CompletionAwareInterface could be used without configuring a subclass of CompletionCommand. Thoughts?

aik099 commented 9 years ago

I had similar idea, while I was deciding on best approach for implementing this.

One of them included what you're proposing. I've discarded it back then because code duplication would be required for completing argument and option (it happens from 2 different methods) and getCompletionHelper is called from both of them so I decided to create special CommandCompletion class to encapsulate the logic.

I'll make proposed changes then, OK?

Sould I also:

aik099 commented 9 years ago

I've did all but Sould I also part and included tests for added code as well.

stecman commented 9 years ago

I'd be happy with completeOptionValues and completeArgumentValues (plural). Also, having a further think about this interface, it makes sense to pass the current CompletionContext to those methods. So:

interface CompletionAwareInterface
{
    public function completeOptionValues($optionName, CompletionContext $context);
    public function completeArgumentValues($argumentName, CompletionContext $context);
}

That PR is looking good now - nice work. With these updates it'll be good to merge :+1:

aik099 commented 9 years ago

I'll make necessary adjustments tomorrow then (it's 00:19 here in Riga, Latvia).

What you think about central completion initialization method per command, that I've described here: https://github.com/stecman/symfony-console-completion/pull/20#issuecomment-71444952 ? I understand that in real life the completeOptionValues/completeArgumentValues will be called only once and only for one command so we can safely make that initialization in these methods and who needs can extract that initialization code to be part of the command and not add it to interface.

By the way I'm in GMT+2 timezone, but you're in GMT+13 so beware that there might be 1 day delay before I can make changes :smile:

aik099 commented 9 years ago

I've updated the PR as requested.