SEPURI-SAI-KRISHNA commented on code in PR #45407:
URL: https://github.com/apache/superset/pull/45407#discussion_r4358381840
##########
superset/utils/excel.py:
##########
@@ -48,12 +48,18 @@
def _quote_formula(value: Any) -> Any:
- """Prefix a string with a quote when it would parse as a formula."""
- return (
- f"'{value}"
- if isinstance(value, str) and len(value) and value[0] in
FORMULA_PREFIXES
- else value
- )
+ """Prefix a string with a quote when it would parse as a formula.
+
+ Leading whitespace is ignored when detecting a formula, because spreadsheet
+ apps still evaluate a cell whose formula prefix is preceded by spaces or
+ tabs (e.g. ``" =cmd"`` or ``"\t=cmd"``). This matches the streaming writer
+ in ``superset.utils.excel_streaming``, which the comment on
+ ``FORMULA_PREFIXES`` says both export paths agree with.
+ """
+ if not isinstance(value, str):
+ return value
+ stripped = value.lstrip()
+ return f"'{value}" if stripped and stripped[0] in FORMULA_PREFIXES else
value
Review Comment:
You are right, and I checked it rather than argue it. Writing the cell types
out with no quoting at all:
```
'=1+1' -> FORMULA cell
' =1+1' -> string cell
'\t=1+1' -> string cell
'+1+1' -> string cell
'@SUM(A1)' -> string cell
' - North' -> string cell
```
So the answer to your question is no: there is no case where the
leading-whitespace form is evaluated in the XLSX output. XlsxWriter converts
only a string whose first character is `=`, which the old guard already caught.
The streaming writer never produces a formula at all, since it goes through
`write_string`. My docstring claimed these values were evaluated, and that was
wrong. I have replaced it.
Your `" - North"` point is also right, and it is worth being exact about
where it already applies:
```
value CSV streaming xlsx before xlsx after
' =1+1' "' =1+1" "' =1+1" ' =1+1' "' =1+1"
' @SUM(A1)' "' @SUM(A1)" "' @SUM(A1)" ' @SUM(A1)' "' @SUM(A1)"
' - North' "' - North" "' - North" ' - North' "' - North"
```
`_starts_like_spreadsheet_formula` in `csv.py` strips, and
`_quote_if_formula` in `excel_streaming.py` strips, so an indented label
already exports with an apostrophe from both of those paths. That is what
@rusackas was pointing at: the tradeoff is not new, this brings the third path
in line with the other two.
`excel_streaming._quote_if_formula` carried the same sentence, and since the
description points at that function I corrected both docstrings rather than
leave one copy of the claim behind. That writer is further from evaluating
anything than this one: every string goes out through `write_string`, so it
never produces a formula cell at all.
So I have kept the change and rewritten the justification to what it
actually is, a consistency fix across the export paths, with the quote
defending against how a downstream viewer or a re-import reads the text rather
than against anything XlsxWriter does. The description now says the same and
carries the table.
If you would still rather the XLSX path not strip, the honest version of
that is to stop all three stripping, since leaving this one alone puts the
inconsistency back. I did not take that on here because it would widen a
one-line fix into a change to the CSV export's behaviour.
One thing I noticed while measuring and did not touch: `csv.py` has
`_is_negative_number`, so `-5` exports bare there, while both XLSX paths quote
it as `'-5`. That divergence predates this PR and is the opposite direction to
the one being fixed. Say the word if you want it in scope and I will open it
separately.
--
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]
