Conversation
All three spreadsheet scripts read a delimited file with
`csv.reader(text.splitlines(), ...)`. The reader does join a quoted cell that
spans lines, but `splitlines()` has already thrown the break away, so the words
on either side of it are glued together:
ID,Note
1,"line one
line two"
inspect sample -> ['1', 'line oneline two']
csv_to_xlsx B2 -> 'line oneline two'
A cell that spans lines is ordinary in an exported sheet -- an address, a
description, a note -- and the conversion writes the damage into the .xlsx it
produces.
Read from `io.StringIO(text, newline="")` instead, which is the form the csv
docs ask for and which keeps the break inside the field.
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. If the CSV parsing change is wrong, conversion could write incorrect cell values into generated XLSX files, and inspection or validation could report misleading results. Reverting stops future bad outputs, while existing files can be regenerated from the original CSV source.
… quoted CSV cell
… quoted CSV cell
kilisamemarisaaa
left a comment
There was a problem hiding this comment.
Make the new fixture's newline encoding explicit on Windows
At head d6bdc8ee6d9323c57878078a94f44dbd37147e36, the new test fails on Windows (Python 3.12.6):
python -m pytest tests/test_builtin_office_skills.py -q
1 failed, 4 passed
At index 1 diff: ['1', 'line one\r\nline two'] != ['1', 'line one\nline two']
Path.write_text(..., encoding="utf-8") uses default newline translation, so the fixture at tests/test_builtin_office_skills.py:82 contains CRLF on Windows. The scripts read bytes and the new StringIO(..., newline="") correctly preserves those CRLF characters, but both assertions require LF.
Please write the fixture with newline="" (or explicit bytes) so its contents match the LF assertions on every platform. Changing only that write locally made this test file pass: 5 passed. Product scripts were unchanged. An explicit CRLF fixture expecting CRLF would also cover the intended preservation behavior, without normalizing away the data being tested.
This is a fixture portability issue, not evidence against the parser fix. Validation was limited to this test file; I did not rerun the full AstrBot suite. The temporary fixture change was removed from my isolated review checkout.
Path.write_text without a newline argument translates "\n" to os.linesep, so on Windows the fixture held CRLF while both assertions required LF, and the test failed there although the parser was doing the right thing: preserving the bytes it was given. The fixture now writes with newline="" so its contents are the same on every platform, and a companion case covers a genuinely CRLF file end to end, from the reader through the workbook cell.
|
You are right, and thank you for running it on Windows. Fixed in
I measured the mechanism rather than assuming it, by writing the same string both ways on this machine: I also took your second suggestion and added One correction to something I would otherwise have implied. That CRLF case does not discriminate the
and with the fix back in place, 6 passed. Run on macOS with Python 3.12.11 in an isolated venv (pytest, pytest-asyncio, openpyxl, python-docx, chardet, xlrd, pandas). I have no Windows host, so the CRLF case is exercised there through an explicitly CRLF fixture rather than through the platform, which is the part your run covered and mine cannot. |
kilisamemarisaaa
left a comment
There was a problem hiding this comment.
Rechecked head fcf78d0 in a fresh detached worktree on Windows / Python 3.12.6. python -m pytest tests/test_builtin_office_skills.py -q: 6 passed. The explicit newline setting fixes the Windows fixture failure, and both LF and CRLF quoted-cell preservation cases pass. Reviewed the updated diff; my fixture-portability concern is resolved. This is focused validation, not a full-suite run. Ruff was unavailable in this Python environment.
All three spreadsheet scripts read a delimited file the same way:
csv.readeraccepts any iterable of strings and does join a quoted cell that spans several of them — butsplitlines()has already removed the break, so what the reader joins is"line one"+"line two"with nothing in between.On a two-row file whose second column holds a note that spans lines:
inspect_workbook.py['1', 'line oneline two']['1', 'line one\n line two']csv_to_xlsx.py→B2'line oneline two''line one\nline two'validate_workbook.pyMeasured on
master(e0aa8d3) by running the three scripts over that file.A cell that spans lines is ordinary in an exported sheet — a postal address, a product description, a comment column — and
csv_to_xlsx.pywrites the damage into the.xlsxit produces, so the loss outlives the tool call. Nothing reports it: the row count and the column count stay correct, because the reader did put the two halves in one field. Only the words are wrong.Modifications / 改动点
io.StringIO(text, newline="")replacestext.splitlines()ininspect_workbook.py,validate_workbook.pyandcsv_to_xlsx.py. That is the form thecsvdocumentation asks for, and it keeps the break inside the field.import iois added to each script; nothing else changes, and a file without a multi-line cell parses byte-identically.三个表格脚本都用
csv.reader(text.splitlines(), ...)读取分隔文件。splitlines()已经把换行删掉了,所以跨行的带引号单元格被拼接时,两边的词会粘在一起:"line one\nline two"变成line oneline two,并且csv_to_xlsx.py会把这个错误写进生成的.xlsx。改为io.StringIO(text, newline=""),即 csv 文档推荐的写法。Screenshots or Test Results / 运行截图或测试结果
test_spreadsheet_skill_keeps_a_line_break_inside_a_quoted_celladded totests/test_builtin_office_skills.py. It runsinspect_workbook.pyandcsv_to_xlsx.pyover the file above and asserts the sample and cellB2.Against the unfixed scripts (
git stashon thescripts/directory only):With the fix:
Both remaining failures are
tests/test_fastapi_v1_dashboard.py::test_config_update_revokes_only_affected_shell_sessions; they fail the same way on a cleanorigin/mastercheckout and are unrelated to this change.ruff checkandruff format --checkare clean on all four files.Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。 (Bug fix, no new feature.)
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
📚 I checked the affected WebUI instructions and screenshots in
docs/zhanddocs/enagainst the changed navigation, page structure, and labels, and updated them in this PR (or explained why no documentation update is needed)./ 我已对照变化后的 WebUI 入口、页面结构和术语,核对并在本 PR 中更新
docs/zh和docs/en的相关操作说明与截图(或说明无需更新文档的原因)。 (No WebUI entry point, page or label changes: this is inside three skill scripts.)🤓 I have ensured that no new dependencies are introduced.
/ 我确保没有引入新依赖库。 (
iois stdlib.)😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Preserve embedded line breaks when spreadsheet skills parse delimited files.
Bug Fixes:
Tests: