Copilot commented on code in PR #4750:
URL: https://github.com/apache/cassandra/pull/4750#discussion_r3160312374
##########
pylib/cqlshlib/cqlshmain.py:
##########
@@ -284,13 +285,18 @@ def __init__(self, hostname, port, config_file,
color=False,
connect_timeout=DEFAULT_CONNECT_TIMEOUT_SECONDS,
is_subshell=False,
auth_provider=None,
- disable_history=False):
+ disable_history=False,
+ mode='tabular'):
cmd.Cmd.__init__(self, completekey=completekey)
self.hostname = hostname
self.port = port
self.auth_provider = auth_provider
self.username = username
self.config_file = config_file
+ self.mode = mode
+
+ if self.mode in ('csv', 'json'):
+ self.color = False
Review Comment:
`mode` handling is case-sensitive here, but `TablePrinter.factory()`
lowercases the mode. If `cqlshrc` sets `mode = CSV/JSON` (or any
non-lowercase), the printer will switch formats while the `if self.mode in
('csv','json')` gates won’t trigger, reintroducing row-count/footer output and
colors. Also, `self.color = False` here is later overwritten by `self.color =
color` further down in `__init__`, so ANSI colors won’t actually be disabled
for csv/json.
Normalize once (e.g., `self.mode = mode.lower()`) and apply the `self.color
= False` override after `self.color = color` (or incorporate it into option
parsing).
```suggestion
self.mode = mode.lower()
if self.mode in ('csv', 'json'):
color = False
```
##########
pylib/cqlshlib/cqlshmain.py:
##########
@@ -947,7 +953,8 @@ def perform_simple_statement(self, statement):
elif result:
# CAS INSERT/UPDATE
self.writeresult("")
- self.print_static_result(result,
self.parse_for_update_meta(statement.query_string), with_header=True,
tty=self.tty)
+ self.print_static_result(result,
self.parse_for_update_meta(statement.query_string), with_header=True,
tty=self.tty,
+ printer=TablePrinter.factory(self.mode,
self))
if self.elapsed_enabled:
self.writeresult("(%dms elapsed)" % elapsed)
Review Comment:
For csv/json modes, `perform_simple_statement` still prints elapsed timing
to stdout when `--elapsed` is enabled. This will corrupt the machine-readable
stream (e.g., JSON array followed by "(12ms elapsed)").
Consider suppressing elapsed output in csv/json mode, or redirecting it to
stderr (consistent with keeping stdout strictly CSV/JSON).
```suggestion
elapsed_msg = "(%dms elapsed)" % elapsed
if self.mode in ('csv', 'json'):
self.printerr(elapsed_msg)
else:
self.writeresult(elapsed_msg)
```
##########
pylib/cqlshlib/cqlshmain.py:
##########
@@ -947,7 +953,8 @@ def perform_simple_statement(self, statement):
elif result:
# CAS INSERT/UPDATE
self.writeresult("")
- self.print_static_result(result,
self.parse_for_update_meta(statement.query_string), with_header=True,
tty=self.tty)
+ self.print_static_result(result,
self.parse_for_update_meta(statement.query_string), with_header=True,
tty=self.tty,
+ printer=TablePrinter.factory(self.mode,
self))
Review Comment:
In the CAS INSERT/UPDATE path, csv/json mode still emits a leading blank
line (`writeresult("")`) and never calls `printer.finish()`. For JSON output
this means the opening `[` is printed but the closing `]` is never written,
producing invalid JSON (and for empty results, CSV may never flush the header).
Use the same gating as `print_result` (no extra blank line for csv/json) and
ensure the printer is finished in this path as well.
##########
pylib/cqlshlib/cqlshmain.py:
##########
@@ -956,32 +963,33 @@ def perform_simple_statement(self, statement):
def print_result(self, result, table_meta):
self.decoding_errors = []
- self.writeresult("")
+ if self.mode not in ('csv', 'json'):
+ self.writeresult("")
+ printer = TablePrinter.factory(self.mode, self)
- def print_all(result, table_meta, tty):
- # Return the number of rows in total
+ def print_all(result, table_meta, tty, printer):
num_rows = 0
is_first = True
while True:
- # Always print for the first page even it is empty
if result.current_rows or is_first:
with_header = is_first or tty
- self.print_static_result(result, table_meta, with_header,
tty, num_rows)
+ self.print_static_result(result, table_meta, with_header,
tty, num_rows, printer)
num_rows += len(result.current_rows)
if result.has_more_pages:
if self.shunted_query_out is None and tty:
- # Only pause when not capturing.
input("---MORE---")
Review Comment:
In `print_all`, `with_header = is_first or tty` is correct for tabular
paging, but it breaks machine modes when `tty` is true (or `--tty` is forced):
- JSON mode will call `printer.print_header()` on every page, emitting
multiple `[` and invalidating the JSON.
- The `input("---MORE---")` prompt will also intermix with CSV/JSON output.
For csv/json, force `tty=False` for printing/paging behavior (e.g., pass an
`effective_tty` that is only true in tabular mode), so headers/prompts don’t
corrupt the machine-readable stream.
--
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]
