Skip to content

Conversation

@zonuexe
Copy link
Contributor

@zonuexe zonuexe commented Jul 11, 2022

refs #732

@zonuexe zonuexe force-pushed the feature/userland-readline-completion branch from c97d27d to 7c4f8da Compare July 11, 2022 17:40
@zonuexe zonuexe force-pushed the feature/userland-readline-completion branch from 7c4f8da to 0e5d28c Compare July 11, 2022 17:44
public function useReadline(): bool
{
return isset($this->useReadline) ? ($this->hasReadline && $this->useReadline) : $this->hasReadline;
return isset($this->useReadline) ? ($this->hasNativeReadline && $this->useReadline) : $this->hasNativeReadline;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe these should also be called "useNativeReadline", but I haven't changed them as they are separate issues from auto-completion.

{
return '.';
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left this adapter for interoperability with existing Hoa completions, but you can also remove HoaAutocompleter and make it directly dependent on Psy\TabCompletion\AutoCompleter. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the benefit of interoperability with existing Hoa completions? it's deprecated and unsupported, and the code here is effectively a stripped-down fork. i'm not sure i see the point :)

/**
* Deactivete auto completer for tab completion.
*/
public function deactivateAutoCompleter(): void;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do nothing for deactivate except native readline().

Copy link
Owner

@bobthecow bobthecow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realistically we're a while from a release where we can introduce backwards compatibility breaks.

For better or worse, we need to maintain compatibility with the current AutoCompleter and Readline interfaces (though i'm not worried about anything in Psy\Readline\Hoa\*, that's an implementation detail not a public API 😛)

public function deactivateAutoCompleter(): void
{
// PHP didn't implement the whole readline API when they first switched
// to libedit. And they still haven't.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉


$command = $term.'tput cols && '.$term.'tput lines';
$tput = Processus::execute($command, false);
$tput = ConsoleProcessus::execute($command, false);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh, sorry about that.

{
return '.';
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the benefit of interoperability with existing Hoa completions? it's deprecated and unsupported, and the code here is effectively a stripped-down fork. i'm not sure i see the point :)

/**
* Activete auto completer for tab completion.
*/
public function activateAutoCompleter(AutoCompleter $autoCompleter): void;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make these two a second interface so we can maintain backwards compatibility for existing Readline API.

Co-authored-by: Justin Hileman <[email protected]>
* @see https://www.php.net/readline_info
*/
public function complete(&$prefix);
public function complete(string $prefix, int $index, array $info);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these were passing by reference because they changed $prefix intentionally so the caller could check prefix length.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants