SEPURI-SAI-KRISHNA commented on code in PR #45407:
URL: https://github.com/apache/superset/pull/45407#discussion_r4356728192
##########
tests/unit_tests/utils/excel_tests.py:
##########
@@ -80,6 +80,28 @@ def test_quote_formulas() -> None:
]
+def test_quote_formulas_with_leading_whitespace() -> None:
+ """
+ Formulas are quoted even when a space, tab, or newline comes first.
+
+ Spreadsheet apps still evaluate a cell whose formula prefix is preceded by
+ whitespace, so the guard must strip before inspecting the first character.
+ The streaming writer (``excel_streaming``) and the CSV writer already do
+ this; this keeps the chart XLSX path in line.
+ """
+ df = pd.DataFrame(
+ {"formula": [" =1+1", "\t=1+1", "\n=1+1", " @SUM(A1)", "normal"]}
Review Comment:
Confirmed, and the guard is doing more work than it was before this PR. The
old `len(value)` tested the original string, which was enough while the first
character was read off that same string. `lstrip()` can empty a string that was
not empty, so an input master handled fine now reaches the subscript empty:
```
value master this PR, no guard
"" "" IndexError: string index out of range
" \t\n" " \t\n" IndexError: string index out of range
" " " " IndexError: string index out of range
```
So the fix introduced the reachable crash and the guard is what prevents it,
which makes it exactly the thing that needed a test. A warehouse returning a
blank or whitespace-only cell is ordinary.
Added `test_quote_formulas_leaves_blank_and_whitespace_only_cells_alone`,
asserting `""`, `" \t\n"` and `" "` come back unchanged. It calls
`quote_formulas` directly as you suggested, since a blank cell read back out of
a sheet comes through as a missing value and would hide the result.
I also ran the rest of the changed function the same way. Four conditions,
all now pinned:
```
drop the stripped guard -> fails the new test
drop lstrip -> fails
test_quote_formulas_with_leading_whitespace
drop the isinstance guard -> fails 10 tests
revert to master's guard -> fails
test_quote_formulas_with_leading_whitespace
```
The description now records the guard change too, which it had not.
--
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]
