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

Configuration for continuation prompt (like PS2 or PROMPT2 in some shells) #183

Open
apstndb opened this issue Aug 17, 2024 · 29 comments
Open

Comments

@apstndb
Copy link
Collaborator

apstndb commented Aug 17, 2024

Sometimes, we want to reuse queries by copy-and-paste, but multi-line prompts make it harder.

spanner> SELECT *
      -> FROM Singers
      -> WHERE SingerId = 1;

It is harder to reuse than indentation like

spanner> SELECT *
         FROM Singers
         WHERE SingerId = 1;

Currently, the prompt string for continuous line is hard coded.
https://github.com/cloudspannerecosystem/spanner-cli/blob/v0.10.4/cli.go#L353

It would be nice to have configuration like PROMPT2 in some shells or bool flag (--only-indent-multiline?).

@apstndb
Copy link
Collaborator Author

apstndb commented Aug 21, 2024

I have implemented the prototype.

@yfuruyama
Copy link
Collaborator

Sorry for the delayed comment.

The reason why I chose -> for the multiline prompt is just because it's used in mysql command. There is no any other good reason for that.

I have used spanner-cli and experienced several times that the current prompt (->) is hard to use for copy/paste, so I'm not hesitate to change it.

Do you have any good example we should keep using -> as a prompt? If not, I think we can just remove it.

@apstndb
Copy link
Collaborator Author

apstndb commented Aug 21, 2024

I'm not confident enough to say that prompt2 is harmful.
It can be more useful in interactive query writing if it can display current processing information.

e.g. %_ of zsh PROMPT2

$ ls '
quote> ' "
dquote> " `
bquote> ` (
> ps ) $(
cmdsubst> ls )

It seems psql has similar functionality(%R)
https://www.postgresql.org/docs/current/app-psql.html#APP-PSQL-PROMPTING

In prompt 2 %R is replaced by a character that depends on why psql expects more input: - if the command simply wasn't terminated yet, but * if there is an unfinished /* ... */ comment, a single quote if there is an unfinished quoted string, a double quote if there is an unfinished quoted identifier, a dollar sign if there is an unfinished dollar-quoted string, or ( if there is an unmatched left parenthesis.

@yfuruyama
Copy link
Collaborator

Thank you for sharing the reference, but still I'm not sure if it's really useful or not.

I checked this blog article about Prompt2 usage for Postgres, but I feel it's more complicated and hard to understand what that prompt means.

Excerpt from the article:

[local] gabe@my_database=# SELECT
[more] - > '
[more] ' > name
[more] ' > '
[more] - > FROM users;

Of course, it depends on the prompt the user sets, but instead of letting the user to choose the prompt, I want to provide the best option that's useful for most of the use cases.

In that sense I still lean toward the option that removes the prompt at all.

@yfuruyama
Copy link
Collaborator

Maybe we can 1) change the default prompt2 as an empty string and 2) provide the command line option to allow users to change the prompt2.

But I'm wondering how many users will use that command line option.

@apstndb
Copy link
Collaborator Author

apstndb commented Aug 21, 2024

I checked this blog article about Prompt2 usage for Postgres, but I feel it's more complicated and hard to understand what that prompt means.

I think the example of that article is not practical because multiline string literals are not popular in regular SQL queries.
Personally speaking, mismatched parentheses in subqueries is a daily problem.

sqlite has similar functionality in continuation prompt.

sqlite> SELECT '
'  ...> 1' FROM (
(x1...> SELECT 2
(x1...> )
   ...> ;

@apstndb
Copy link
Collaborator Author

apstndb commented Aug 21, 2024

After investigating, I think parser status in continuation prompt is common and useful feature in SQL interactive shells in ideal, but it is not low hanging fruits.

I'm worried that removing the continuation prompt completely in default will confuse users because they won't know if they're still in the middle of typing.
So I think that adding --prompt2 option and keep current behavior in default is a conservative choice.

@apstndb apstndb changed the title Configuration for multi-line prompt (like PS2 or PROMPT2 in some shells) Configuration for continuation prompt (like PS2 or PROMPT2 in some shells) Aug 21, 2024
@apstndb
Copy link
Collaborator Author

apstndb commented Aug 21, 2024

After investigating, I think parser status in continuation prompt is common and useful feature in SQL interactive shells in ideal, but it is not low hanging fruits.

Currently, the statement separator (gsqlsep) only considers quotes and comments, so some change is required to show if they are inside parentheses.

@apstndb
Copy link
Collaborator Author

apstndb commented Aug 22, 2024

I have come to the following thought:

  • Without the ability to show parser status, there is no particular reason to customize the continuation prompt string.
  • People who care about copy-and-paste will probably use ~/.spanner_cli.cnf to default the continuation prompt to just whitespace.
  • Given the conventions of interactive shells, it would be nice for users to see the continuation prompt (->) by default.

So I think spanner-cli needs a boolean flag to set the continuation prompt to whitespace.
Typical use case:

  • no_prompt2 = true in .spanner_cli.cnf
  • merely used --no-prompt2=false to override setting in spanner_cli.cnf

@yfuruyama
Copy link
Collaborator

Adding the option to disable the prompt2 is fine, but I still doubt that it can address the user's pain points when they want to copy/paste the multi-line queries. This is because there is less chance the (light) users will be aware of the existence of the option and they might keep using the default prompt.

This is not an urgent task, so let's keep this discussion open and here the feedback from other users as well.

@apstndb
Copy link
Collaborator Author

apstndb commented Aug 22, 2024

I suspect that feedback on issues is biased towards only heavy users.

I think we need to go back to why continuation prompts exist as a convention.

I don't yet do deep research about retionale of PROMPT2, but I think this question is an example of the need for continuation prompts for novice users.
https://stackoverflow.com/questions/19710968/sqlite3-prompting-instead-of-sqlite
Novice users (and sometimes I) can forget to input terminating semicolon(;).

Without a continuation prompt, a novice user would not aware that spanner-cli was continuing to read and wait for something.

spanner> SELECT 1
                 
                 broken?
                 ^C
spanner> SELECT 1
             -> 
             -> (What is wanted? Let me see...)

I found that PostgreSQL ML contains a similar topic.
https://www.postgresql.org/message-id/[email protected]

@apstndb
Copy link
Collaborator Author

apstndb commented Aug 22, 2024

MySQL document also have many sentence about the prompt.
https://dev.mysql.com/doc/refman/8.4/en/entering-queries.html

@yfuruyama
Copy link
Collaborator

I understand it's not a good idea to remove the continuation prompt by default as it confuses users when the terminating semicolon is not entered.

Given that removing the continuation prompt likely confuses users (even heavy users as well), I feel it's not also a good idea to provide the option to remove the continuation prompt.

If there is a situation when a user wants to copy the multi-line query itself, it can be accomplished by looking back the query history (Ctrl-P or key) and get the folded single-line query. It's not a perfect solution as it cannot copy the query result together, but I think it can be used as a workable solution.

@apstndb
Copy link
Collaborator Author

apstndb commented Sep 1, 2024

I'm glad to agree abount the default continuation prompt.

Given that removing the continuation prompt likely confuses users (even heavy users as well), I feel it's not also a good idea to provide the option to remove the continuation prompt.

I feel that prompt2 in ini file can be "Shooting Yourself in the Foot", but it is useful in some situation and it is not strange to have conguration like that.

If there is a situation when a user wants to copy the multi-line query itself, it can be accomplished by looking back the query history (Ctrl-P or key) and get the folded single-line query. It's not a perfect solution as it cannot copy the query result together, but I think it can be used as a workable solution.

Currently, line edit history only get a single line of a past query.
If it will be changed to get a folded query, non-trivial problem is happened if newline have some meaning like:

  • comments
  • quoted string or literals

It's afraid that it silently changes the meaning of a query.
I feel it is more serious problem than copy-and-paste.

@yfuruyama
Copy link
Collaborator

yfuruyama commented Sep 3, 2024

I feel that prompt2 in ini file can be "Shooting Yourself in the Foot", but it is useful in some situation and it is not strange to have conguration like that.

Yes, it's not strange to have that option, but the only concern I have is it likely becomes "yet another" option that might be used by only a few users in a limited situation, and we have to keep maintaining it.

Of course we already have many optional options (10 options) in spanner-cli and I'm struggling to keep the balance for what we should add/shouldn't add to keep the tool simple.

It's afraid that it silently changes the meaning of a query. I feel it is more serious problem than copy-and-paste.

I think it's not so serious because I don't think many people enter comments or multi-line literals in an interactive way. If it's really serious, we have to change even the current implementation with stopping showing the query history when multi-line query is used, but I haven't heard of such issue yet.

@yfuruyama
Copy link
Collaborator

One random idea to avoid adding a new option is making a new prompt variable in --prompt option that disables the continuous prompt.

For example, when a user specify --prompt='spanner-cli\t>\c' (\c indicates disabling the continuous prompt, for example), it stops showing the continuous prompt.

The downside is that it's not intuitive, but given that it might be used by only a few (heavy) users in a limited situation, that might be acceptable.

It's just a random thought though.

@apstndb
Copy link
Collaborator Author

apstndb commented Sep 4, 2024

Yes, it's not strange to have that option, but the only concern I have is it likely becomes "yet another" option that might be used by only a few users in a limited situation, and we have to keep maintaining it.
Of course we already have many optional options (10 options) in spanner-cli and I'm struggling to keep the balance for what we should add/shouldn't add to keep the tool simple.

I think it may be a problem to keep --help simple.
This can be partially resolved by making prompt2 an option that can only be specified in the ini file and not from the command line.

Prompt2      *string `ini-name:"prompt2" description:"Set the prompt2 to the specified format"`

psql's PROMPT2 is not a command line option, it is a variable can be set:

  • \set PROMPT2 value in .psqlrc (can be set interactively)
  • --variable=PROMPT2=value

I think it's not so serious because I don't think many people enter comments or multi-line literals in an interactive way. If it's really serious, we have to change even the current implementation with stopping showing the query history when multi-line query is used, but I haven't heard of such issue yet.

I think this is a related but independent problem to prompt2,
multi-line editing and multi-line history problem.

  • Users often executes written queries, which may have comments, in interactive tool.
    • Even when the -f and --file= flags are appropriate.
  • If the history is executable, users expects this meaning must not be changed.
  • The executable history will be a target of reedit and reuse.

Current history implementation shows every unexecutable line flagment for multi-line queries.
This is noise in the history, but it's not dangerous.

My thought:

  • Current behavior(show every line history)
    • I feels it is unnoying but not dangerous.
  • Show multi-line query as folded single line.
    • I am doubting it is dangerous in some situation.
  • Stop to show history of multi-line query.
    • I think It is safe and better than current behavior.
    • Sometimes history-back doesn't show the previous query.
      • This behavior may surprise users.
  • Natively support multi-line editing with history.

@apstndb
Copy link
Collaborator Author

apstndb commented Sep 4, 2024

One random idea to avoid adding a new option is making a new prompt variable in --prompt option that disables the continuous prompt.

It is a possible implementation.

If the ini only option is acceptable, I think it is no reason to extend prompt to disable the continuous prompt.
I'm a bit curious to know how much users are using the --prompt command line option rather than the prompt setting in .spanner_cli.cnf.

@apstndb
Copy link
Collaborator Author

apstndb commented Sep 4, 2024

Random thought
To avoid the explosion of command line options, it may be time to introduce the concept of system variables.

@yfuruyama
Copy link
Collaborator

This can be partially resolved by making prompt2 an option that can only be specified in the ini file and not from the command line.

It's better than providing nothing, so +1 on this change as an initial implementation.

@apstndb
Copy link
Collaborator Author

apstndb commented Sep 4, 2024

I'm glad we agreed on the prompt2 ini option choice.

I have noticed there are some design choices about prompt2 indentation.

  1. Align to prompt width in default
  2. No default alignment

Current behavior is 1 in all situation, but there is a corner case.

If prompt is long, simple query can reach the line end easily.

Assumes documented prompt = "[\\p:\\i:\\d]\\t> " and terminal width is 80 characters(old example!).
スクリーンショット 2024-09-05 0 06 15

I am doubting if the default padding is useful.

It is not bad without padding.
スクリーンショット 2024-09-05 0 15 44

Proposed behavior

  • No default padding.
  • The default prompt2 value is " -> ", it is the string of the same length as the default prompt spanner> .
  • There is a room to add psql style padding %w(whitespaces of the same length as the prompt), or more magical left padding.

@yfuruyama
Copy link
Collaborator

My thoughts about the prompt alignment are

  • The main use case for disabling prompt 2 is making it easy to copy & paste the multi-line query, so default alignment is not needed if the prompt2 is set with empty string ("").
  • For other use cases (mainly for default users), the readability is more important and I think it's more readable if the multi-line query is aligned with default alignment (i.e. keep current behavior).

@apstndb
Copy link
Collaborator Author

apstndb commented Sep 5, 2024

For other use cases (mainly for default users), the readability is more important and I think it's more readable if the multi-line query is aligned with default alignment (i.e. keep current behavior).

I assume that readability can be compromised when the prompt is long.
Consider the case where project, instance and database have reached their maximum length.

Project ID: 30 characters
Instance ID: 60 characters
Database ID: 30 characters

If spanner-cli always right align continuation prompt to first prompt line, \p\i\d and prompt2 produces 120 characters, and there is very little room to write a query.

There are some options.

  1. User modify prompt not to use prompt variables.
    • Current possible workaround
    • Information provided by prompt variables is useful to avoid operation mistake, so it is not ideal.
  2. User modify prompt2 to ``
    • A possible solution for your expected behavior without modifying prompt.
    • Continuation prompt is useful as discussed, so it is not ideal.
  3. Support multi-line prompt and right align to the last line.
    • It doesn't answer general long prompt, but it may be nice for user want many information in prompt.
  4. Never align prompt2
    • It won't satisfy your expectation.
  5. Let user choose the alignment
    • It makes prompt2 more complicated, but I expect it can satisfy all user's expectation.
    • I think it is similar to the spec discussed in pgsql-hackers

      Define it as "difference between PROMPT1's width and the total width
      of non-%w elements in this prompt".

FYI: I can't found other implementation which support prompt configuration and default alignment.

psql

postgres=# \set PROMPT1 veryyyyyyyyyyyyyyylooooooooooooooooooonnnng=#
veryyyyyyyyyyyyyyylooooooooooooooooooonnnng=#SELECT 1
postgres-# 
postgres-# 
postgres-# ;

sqlite

sqlite> .prompt veryyyyyyyyyyyyyyylooooooooooooooooooonnnng>
veryyyyyyyyyyyyyyySELECT 1
   ...> 
   ...> 
   ...> ;

mysql

mysql: It doesn't support prompt configuration.

mysql> prompt veryyyyyyyyyyyyyyylooooooooooooooooooonnnng>
PROMPT set to 'veryyyyyyyyyyyyyyylooooooooooooooooooonnnng>'
veryyyyyyyyyyyyyyylooooooooooooooooooonnnng>SELECT 1
    -> 
    -> 
    -> ;

So, I am not sure whether your expectation is common sense to other users.

@yfuruyama
Copy link
Collaborator

yfuruyama commented Sep 5, 2024

Maybe "readability" was not good word. Let me rephrase it.

spanner-cli is an interactive CLI, so I assume that users write multi-line query in real time. When user writes multi-line query from the first line to the second line, if the prompt is not aligned, the user has to move their eyes back and forth to check whole query. This process continues until the whole query is constructed. This is apparently not good for usability.

I think the example in the last reply is the edge case and doesn't reflect the real use case. If it's difficult to write a multi-line query with prompt alignment due to short space, the same is true for the single-line query. So I guess that nobody use such a long project/instance/database names or long prompt.

Anyway, instead of having a discussion around an edge case, let's focus on solving the core issue we have discussed - unable to copy/paste multi-line query. To solve that, what we have discussed are:

  • Allow users to disable continuous prompt in ini file as prompt2 variable.
  • No extra space for continuous prompt if user sets an empty string to prompt2 so that multi-line query is easy to be copied.

@apstndb
Copy link
Collaborator Author

apstndb commented Sep 6, 2024

OK, we should talk about a concrete solution.

Allow users to disable continuous prompt in ini file as prompt2 variable.

  • Add prompt2(string) and no_prompt2_alignment(bool) ini option.
  • The default is prompt2="-> " and no_prompt2_alignment=false to continue current behavior.
  • If user want to disable the default alignment, no_prompt2_alignment=true will be set.

I think it is a possible solution for all personas. What do you think?

@apstndb
Copy link
Collaborator Author

apstndb commented Sep 6, 2024

I want to fork some problem to another issue.

I think the example in the last reply is the edge case and doesn't reflect the real use case. If it's difficult to write a multi-line query with prompt alignment due to short space, the same is true for the single-line query. So I guess that nobody use such a long project/instance/database names or long prompt.

I guess it is simply because prompt configuration with prompt variables is a rarely used feature.
I have seen many project/instance/database ids which are same pattern like {company}-{service}-{environment} by convention.
I believe prompt variables are helpful feature, and if some users don't use this feature to avoid longer prompt, I believe newlines in prompt will solve this problem.

#186

@apstndb
Copy link
Collaborator Author

apstndb commented Sep 28, 2024

I don't know if we've reached an agreement.

My proposal

  • Add prompt2(string) and no_prompt2_alignment(bool) ini option.
  • The default is prompt2="-> " and no_prompt2_alignment=false to continue current behavior.
  • If user want to disable the default alignment, no_prompt2_alignment=true will be set.

@yfuruyama
Copy link
Collaborator

Sorry for the delayed response.

As I commented at #183 (comment), I don't think we need to allow users to customize the prompt alignment.

Also my proposal at that comment was providing prompt2 variable, but more direct option to disable the prompt2 would be user-friendly as there is no useful use case to change the prompt2 string.

So my updated suggestion is:

  • Allow users to disable continuous prompt in ini file with no_prompt2 option.
  • No extra space for continuous prompt if user sets no_prompt2, so that multi-line query is easy to be copied.

@apstndb
Copy link
Collaborator Author

apstndb commented Sep 30, 2024

Since there seems to be no consensus on implementing prompt2 again, I don't think there is any need to discuss customizing prompt2 any further.
I'll use my own build with my patchset if it is needed.

I have no -1 to your suggestion as an official feature for spanner-cli.

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 a pull request may close this issue.

2 participants