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

Make cli handle multiple whitespaces #1388

Merged
merged 1 commit into from
Dec 2, 2021

Conversation

capkurmagati
Copy link
Contributor

Which issue does this PR close?

The cli cannot handle whitespace like

❯ \?
'\? ' is not a valid command
❯ \quiet true
'\quiet true ' is not a valid command

The PR fixes this issue by removing the consecutive whitespaces.

Closes #.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the datafusion Changes in the datafusion crate label Dec 1, 2021
@@ -90,7 +90,8 @@ pub async fn exec_from_repl(ctx: &mut Context, print_options: &PrintOptions) {
match rl.readline("❯ ") {
Ok(line) if line.starts_with('\\') => {
rl.add_history_entry(line.trim_end());
if let Ok(cmd) = &line[1..].parse::<Command>() {
let command = line.split_whitespace().collect::<Vec<_>>().join(" ");
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem I'd like to fix here is not only leading and trailing whitespaces.
Currently, the behavior is like

DataFusion CLI v5.1.0

❯ \quiet  true
'\quiet  true' is not a valid command
❯ \pset  format  csv
'\pset  format  csv' is not a valid command
❯ \pset format  csv
Execution error:  csv is not a valid format type [possible values: csv, tsv, table, json, ndjson]

Without normalizing whitespaces before parsing commands, every command need to do trim like here.
https://github.com/apache/arrow-datafusion/blob/42f8ab1e44510cea73f9eb2300d28f2fa92ac55a/datafusion-cli/src/functions.rs#L159

Copy link
Member

Choose a reason for hiding this comment

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

oh i see, thanks for the context.

@houqp
Copy link
Member

houqp commented Dec 2, 2021

eventually, we will need to write a proper lexer and parser for pgcli commands :P

@houqp houqp added the bug Something isn't working label Dec 2, 2021
@houqp houqp requested a review from jimexist December 2, 2021 06:22
@alamb alamb merged commit 069449f into apache:master Dec 2, 2021
@alamb
Copy link
Contributor

alamb commented Dec 2, 2021

Thanks @capkurmagati -- I think this is a small enough fix we can iterate on it if @jimexist has some suggestions as well

@capkurmagati capkurmagati deleted the cli-handle-whiteshpaces branch December 3, 2021 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working datafusion Changes in the datafusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants