dariocurr commented on code in PR #23797:
URL: https://github.com/apache/datafusion/pull/23797#discussion_r3650045330
##########
datafusion-cli/src/exec.rs:
##########
@@ -129,13 +130,15 @@ pub async fn exec_from_files(
pub async fn exec_from_repl(
ctx: &dyn CliSessionContext,
print_options: &mut PrintOptions,
+ history_file: Option<&Path>,
Review Comment:
Took this suggestion and pushed an update: `exec_from_repl` now takes a
`ReplOptions` struct (in its own new module,
`datafusion-cli/src/repl_options.rs`, mirroring how `PrintOptions` gets its own
module) instead of the raw `Option<&Path>`.
I kept it as a plain, exhaustively-constructed struct rather than adding
`#[non_exhaustive]` + partial-construction-via-`Default`, so it stays
consistent with `PrintOptions`'s existing convention in this same crate (plain
struct, named-field construction at each call site) rather than introducing a
second, different pattern for "options" structs. So this doesn't fully close
the door on future signature churn the way your suggestion would, but it does
give us a dedicated place to add REPL-specific options later without touching
`PrintOptions`, and it let me centralize both the `.history` default and the
`DATAFUSION_HISTORY_FILE` env var resolution into `ReplOptions::default()`
instead of duplicating that logic across callers. Thanks for the pointer to the
pattern either way!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
