Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented input completion for Userland #733

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

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
@@ -764,7 +777,7 @@ public function setUseReadline(bool $useReadline)
*/
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 😛)

src/Configuration.php Outdated Show resolved Hide resolved
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.

🎉

@@ -120,7 +120,7 @@ public static function getSize(): array
}

$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]>
*/
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