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.

Sign up for GitHub

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

Jump to bottom

Add autosuggestion to cli #11671

Merged
merged 1 commit into from
May 11, 2022
Merged

Add autosuggestion to cli #11671

merged 1 commit into from
May 11, 2022

Conversation

aakashnand
Copy link
Member

@aakashnand aakashnand commented Mar 25, 2022 •
edited by ebyhr
Loading

Description

trino-auto-suggestion

Added autosuggestion to trino cli.

Is this change a fix, improvement, new feature, refactoring, or other?

new feature

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

client library

How would you describe this change to a non-technical end-user or system administrator?

N/A

Related issues pull requests and links

NA

Documentation

() No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries are required.
(x) Release notes entries required with the following suggested text:

# CLI
* Add support for auto suggestion from history. This can be disabled by `--disable-auto-suggestion` parameter.

@cla-bot cla-bot bot added the cla-signed label Mar 25, 2022
ebyhr
ebyhr reviewed Mar 26, 2022
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Looks useful. Can we document the key binding in cli.rst?

@ebyhr ebyhr requested a review from electrum March 26, 2022 00:36
@aakashnand
Copy link
Member Author

@ebyhr sure. I will add the documentation as well. Should I do that in different PR?

@ebyhr
Copy link
Member

ebyhr commented Mar 26, 2022

@aakashnand You can include the document in this PR.

@aakashnand
Copy link
Member Author

Understood will do it soon

@github-actions github-actions bot added the docs label Mar 26, 2022
@aakashnand
Copy link
Member Author

@ebyhr Added documentation in cli.rst

@findepi findepi removed their request for review March 28, 2022 07:30
@Ordinant
Copy link
Member

Ordinant commented Apr 7, 2022

(x) No documentation is needed.

Please remove this X and place it on the next entry, "Sufficient doc..."

This feature could also use a release note to say that the feature is available as of release whatever. It won't take much: "The Trino CLI now autocompletes commands based on the history of commands already entered."

I would also like to see an option to disable this feature in case an individual user finds it distracting or confusing or just hates it.

Ordinant
Ordinant reviewed Apr 7, 2022
View reviewed changes
docs/src/main/sphinx/installation/cli.rst Outdated
@@ -160,6 +160,14 @@ press :kbd:`Enter`.
By default, you can locate the Trino history file in ``~/.trino_history``.
Use the ``TRINO_HISTORY_FILE`` environment variable to change the default.

Autosuggestion
--------------
As you type commands in CLI , you will see a completion offered after the cursor
Copy link
Member

@Ordinant Ordinant Apr 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

... in the CLI, you see offered a suggested completion in muted gray ...

[no future tense in docs]

@aakashnand
Copy link
Member Author

@Ordinant thank you for comments. I will work on them. For disabling autosuggestion what do you think about adding one more CLI option like --disable-autosuggestion?

@Ordinant
Copy link
Member

Ordinant commented Apr 7, 2022 •
edited
Loading

Once this feature gets into the field and people like it, I would love to see the analogous feature added to upstream editors.

@Ordinant
Copy link
Member

Ordinant commented Apr 7, 2022

@aakashnand, yes, we don't have properties here for the CLI (although that's not a bad future idea). So the option you suggest looks fine to me.

@aakashnand aakashnand force-pushed the autosuggestion branch 2 times, most recently from 9df295f to ad3d8a6 Compare April 7, 2022 05:44
@aakashnand aakashnand requested a review from Ordinant April 7, 2022 05:45
@aakashnand
Copy link
Member Author

@Ordinant I added the option you suggested. Now user can disable the autosuggestion using --disable-autosuggestion

OPTIONS:
      --access-token=<token> Access token
      --catalog=<catalog>    Default catalog
      --client-info=<info>   Extra information about client making query
      --client-request-timeout=<timeout>
                             Client request timeout (default: 2m)
      --client-tags=<tags>   Client tags
      --debug                Enable debug information
      --disable-autosuggestion
                             Disable autosuggestion
      --disable-compression  Disable compression of query results

autosuggestion-disable-option

@aakashnand aakashnand force-pushed the autosuggestion branch 2 times, most recently from 68d3357 to 9cd4c6b Compare April 10, 2022 14:10
@aakashnand
Copy link
Member Author

@ebyhr @Ordinant @electrum any idea when can this be merged? Any further changes required?

@aakashnand aakashnand force-pushed the autosuggestion branch 2 times, most recently from 4a78458 to 9f0e5d0 Compare April 14, 2022 07:37
@aakashnand
Copy link
Member Author

@ebyhr any idea why 1 check is failing?

@ebyhr
Copy link
Member

ebyhr commented Apr 14, 2022

@aakashnand It comes form lack of 377 docker image. You can ignore the failure.

Related Slack post https://trinodb.slack.com/archives/CFP480UKX/p1649927228980439?thread_ts=1649910849.504189&cid=CFP480UKX

@aakashnand aakashnand force-pushed the autosuggestion branch 2 times, most recently from 1c180e6 to 104718d Compare April 21, 2022 09:12
@aakashnand
Copy link
Member Author

@electrum I have rebased this again, any idea when can we merge this?

ebyhr
ebyhr approved these changes May 11, 2022
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Could you rebase on upstream to resolve a conflict?

docs/src/main/sphinx/installation/cli.rst Outdated Show resolved Hide resolved
@aakashnand aakashnand requested a review from ebyhr May 11, 2022 06:03
@aakashnand
Copy link
Member Author

@ebyhr Please do the final review one last time 🙏🏼 . Hope this gets merged soon Waited for too long time

@ebyhr ebyhr merged commit a48b876 into trinodb:master May 11, 2022
@ebyhr ebyhr mentioned this pull request May 11, 2022
Closed
@github-actions github-actions bot added this to the 381 milestone May 11, 2022
@mosabua mosabua mentioned this pull request May 11, 2022
Merged
mosabua
mosabua reviewed May 11, 2022
View reviewed changes
docs/src/main/sphinx/installation/cli.rst
@@ -428,6 +430,14 @@ mode:

.. _cli-output-format:
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be with the outputformats header @colebow will take care of this

mosabua
mosabua reviewed May 11, 2022
View reviewed changes
docs/src/main/sphinx/installation/cli.rst
@@ -428,6 +430,14 @@ mode:

.. _cli-output-format:

Auto suggestion
---------------
As you type commands in the CLI , you see offered a suggestion after the cursor
Copy link
Member

Choose a reason for hiding this comment

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

Empty line after title

Copy link
Member

Choose a reason for hiding this comment

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

@colebow will send PR with fixes

@aakashnand
Copy link
Member Author

aakashnand commented May 12, 2022 •
edited
Loading

@mosabua @ebyhr I have marked checkbox for release notes as well as documentation but it is not coming up in 381 release notes, is something wrong? #12277

@ebyhr
Copy link
Member

ebyhr commented May 12, 2022

@aakashnand See https://github.com/trinodb/trino/pull/12277/files#diff-0617d9fe47a2044579f87a4928098c078f843100d0e13bae30d6ac6473cd7a08R34-R37

@aakashnand
Copy link
Member Author

got it. I was confused because of these red crosses

image

@mosabua
Copy link
Member

mosabua commented May 12, 2022

We are still updating wording for that and need that confirmed. And @colebow will also send a PR to update the docs some more.

@colebow colebow mentioned this pull request May 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mosabua mosabua mosabua left review comments

@Ordinant Ordinant Ordinant left review comments

@electrum electrum Awaiting requested review from electrum

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees
No one assigned
Labels
cla-signed docs
Milestone
381
Development

Successfully merging this pull request may close these issues.
4 participants
@aakashnand @ebyhr @Ordinant @mosabua