Skip to content

Add autosuggestion to cli#11671

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
aakashnand:autosuggestion
May 11, 2022
Merged

Add autosuggestion to cli#11671
ebyhr merged 1 commit intotrinodb:masterfrom
aakashnand:autosuggestion

Conversation

@aakashnand
Copy link
Copy Markdown
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
@aakashnand aakashnand requested review from ebyhr and findepi March 25, 2022 16:33
ebyhr
ebyhr reviewed Mar 26, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member Author

aakashnand commented Mar 26, 2022

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

@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Mar 26, 2022

@aakashnand You can include the document in this PR.

@aakashnand
Copy link
Copy Markdown
Member Author

aakashnand commented Mar 26, 2022

Understood will do it soon

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

aakashnand commented Mar 26, 2022

@ebyhr Added documentation in cli.rst

@findepi findepi removed their request for review March 28, 2022 07:30
@Ordinant
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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]

Ordinant
Ordinant reviewed Apr 7, 2022
View reviewed changes
Ordinant
Ordinant reviewed Apr 7, 2022
View reviewed changes
Ordinant
Ordinant reviewed Apr 7, 2022
View reviewed changes
@aakashnand
Copy link
Copy Markdown
Member Author

aakashnand commented Apr 7, 2022

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

aakashnand commented Apr 7, 2022

@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

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

aakashnand commented Apr 14, 2022

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

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

aakashnand commented Apr 14, 2022

@ebyhr any idea why 1 check is failing?

@ebyhr
Copy link
Copy Markdown
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
Copy Markdown
Member Author

aakashnand commented Apr 21, 2022

@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
Copy Markdown
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?

@aakashnand aakashnand requested a review from ebyhr May 11, 2022 06:03
@aakashnand
Copy link
Copy Markdown
Member Author

aakashnand commented May 11, 2022

@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
Copy Markdown
Member

@mosabua mosabua May 11, 2022

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

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

@mosabua mosabua May 11, 2022

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
Copy Markdown
Member

@mosabua mosabua May 11, 2022

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
Copy Markdown
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
Copy Markdown
Member

ebyhr commented May 12, 2022

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

@aakashnand
Copy link
Copy Markdown
Member Author

aakashnand commented May 12, 2022

got it. I was confused because of these red crosses

image

@mosabua
Copy link
Copy Markdown
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

@electrum electrum Awaiting requested review from electrum

@ebyhr ebyhr Awaiting requested review from ebyhr

+1 more reviewer

@Ordinant Ordinant Ordinant left review comments

Reviewers whose approvals may not affect merge requirements

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