-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
#1268: allow datafusion-cli to toggle quiet flag within CLI #1330
Conversation
6ee2a31
to
08b77c3
Compare
@jgoday you need address the conflicts. |
datafusion-cli/src/command.rs
Outdated
@@ -117,6 +121,7 @@ impl FromStr for Command { | |||
("d", None) => Self::ListTables, | |||
("d", Some(name)) => Self::DescribeTable(name.into()), | |||
("?", None) => Self::Help, | |||
("quiet", Some(b)) => Self::QuietMode(b == "true" || b == "t"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the doc, we just accept true/false, i think this code is inconsistent with the doc.
Maybe we should consider the lower/upper case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, in psql it's common to have on|off flags (case insensitive), should we do the same ?
\x [on|off|auto] toggle expanded output (currently off)
08b77c3
to
be98286
Compare
Ok, rebased to master to avoid conflicts |
datafusion-cli/src/exec.rs
Outdated
loop { | ||
match rl.readline("❯ ") { | ||
Ok(line) if line.starts_with('\\') => { | ||
rl.add_history_entry(line.trim_end()); | ||
if let Ok(cmd) = &line[1..].parse::<Command>() { | ||
match cmd { | ||
Command::Quit => break, | ||
Command::QuietMode(quiet) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why not move the execution to the command file? quit should be the only exception here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair question, but I did not dare to change the Command::execute signature (to make print_options mutable).
What do you think about that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe it's used outside this repo nor any downstream code so you should feel free to change it as you see fit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok then, changed to delegate the mutation to command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Which issue does this PR close?
Closes #1268.
Rationale for this change
Allow to toggle quiet mode inside de datafusion-cli.
What changes are included in this PR?
Adds a datafusion-cli command (QuietMode bool) to change the print_options quiet mode in runtime.
Are there any user-facing changes?
A new command is introduced into the datafusion-cli repl.
Updated docs/source/user-guide/cli.md