Make all completion related method return empty array for no completions

[aik099]

I've also changed PHPDoc of CompletionInterface (removed null part) because I belive that method return type should be consistent. If we're returning possible option/argument values, then no values means empty array instead special null to indicate that.

[stecman]

I'm not sure if this is related to the suggestion for CompletionHandler::run to return an array, but if it is I had something slightly different in mind and I've made that change in a836d47.

Considering this as a separate idea, I'm all for consistency, but there are definitely two separate responses each completeFor* function has:

1. Success: "This is the correct mode of completion, here are the results" (return array)
2. Failure: "This is not the correct mode of completion" (return false)

These are internal methods, so I'm happy for them to behave like this. I find that (bool) array() === false muddies the intent somewhat, but more importantly the loop through completion modes is actually supposed to stop after the first one matches: returning an array with any number of elements. I've fixed this in e511e0e.

[aik099]

This is not the correct mode of completion

What this mean? Each of completion methods can return results. Yes there can be 0 results too. Whatever or not it's important to know for the caller the fact, that particular completion method doesn't apply here (e.g. complete for options called before command was detected) I'm not sure.

These are internal methods, so I'm happy for them to behave like this. I find that (bool) array() === false muddies the intent somewhat, but more importantly the loop through completion modes is actually supposed to stop after the first one matches: returning an array with any number of elements. I've fixed this in e511e0e.

I guess I need to rebase and see what's left of this PR.

Of what I'm sure at 100% is that CompletionInterface::run should return an array. Accepting both empty array and null as no suggestions just doesn't sound right.

[aik099]

I've updated/rebased the PR. What's left is:

1. the completeOption now also returns false if no viable completion sources (helper/command) were found
2. the CompletionInterface::run now returns array
3. added some DocBlock to fix PhpStorm auto-completion of getAllOptions method result

[stecman]

Sorry to keep coming back on this, but all of that casting is a bit unnecessary in my mind - the same behaviour can be achieved more cleanly with just:

public function runCompletion()
{
    ...

    foreach ($process as $methodName) {
        $result = $this->{$methodName}();

        if (is_array($result) || $result) {
            // Return the result of the first completion mode that matches
            return (array) $this->filterResults($result);
        }
    }

    ...
}

The other changes are all good.

[aik099]

Sorry to keep coming back on this

That's ok. You haven't seen comment count on PR's I'm reviewing at https://github.com/qa-tools/qa-tools/pulls?q=is%3Apr+is%3Aclosed (some have 99+ comments).

but all of that casting is a bit unnecessary in my mind - the same behaviour can be achieved more cleanly with just:

So we don't cast result of CompletetionInterface::run and calls to new methods to get option/argument completions right from command, but instead make more advanced check before we use them. Understood.

[aik099]

Unfortunately without type casing making call to filterResults which accepts only arrays will end up in fatal error.

[aik099]

Can you please specify exact lines in diff where you think typecasting can be avoided and why.

[stecman]

Ah right, missed that - just move the cast to the parameter:

return $this->filterResults((array) $result);

Can you please specify exact lines in diff where you think typecasting can be avoided and why.

return (array) $helper->run();
return (array) $this->command->completeOptionValues($option->getName(), $this->context);
return (array) $this->command->completeArgumentValues($option->getName(), $this->context);

It's the casting of user provided values in multiple places when they are only used in one place. The CompletionHandler::completeFor* methods are internal and are only called from CompletionHandler::runCompletion, so there is no benefit in repeating the cast inside each method when it can be done once higher up the chain. If these methods were part of the public API I would probably want to guarantee the return type, but since they're not it's just code for the sake of code IMO.

Additionally, the casting inside Completion::run is sort of redundant as you're also casting when it's being called later return (array) $helper->run(). Technically I guess Completion is part of the public API, but the internal cast isn' really necessary (I'd prefer just to indicate the return type of CompletionInterface::run as array|string).

A single cast in CompletionHandler:runCompletion seems like the ideal solution to me at the moment, but if anyone can give a good argument for a different approach I'm all ears.

[aik099]

I'd prefer just to indicate the return type of CompletionInterface::run as array|string

Are you sure? The output of this method isn't then exploded by \n if it's not an array. Returning a string will you get you array('') after casting.

Other changes done. I've also changed check in runCompletion method for it check that suggestion result is not false specifically (because we now return false for internal completion methods in case, when it can't be executed to provide any useful completions (e.g. argument name completions, when we expect option names).