fix: handle non-Text nodes in Text.__getitem__ - #352
kshivam4781 wants to merge 2 commits into
Conversation
node_size computation only handled str via sizeof() and fell back to len(node) for everything else, but Text.__init__ accepts arbitrary NodeType (Any) and render() converts any non-Text node via str(node). len(node) then raised TypeError for int/float/etc. Fixes K1rL3s#305.
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughИзменена нарезка ChangesНарезка
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Описание
Text.__getitem__raisedTypeErrorfor any node that was neitherstrnorText(e.g.int,float), even thoughText.__init__accepts arbitraryNodeType(= Any) andrender()already treats any non-Textnode asstr(node):The size computation only branched on
strvs "everything else" and calledlen(node)for non-strnodes, which fails for objects (likeint) that don't implement__len__.Note on the fix vs. the issue's suggested one-liner:
node_size = len(node) if isinstance(node, Text) else sizeof(str(node))alone isn't quite enough, because the slicing branch further down (new_node = node[a:b]for the non-strcase) would still raise on a non-Textnode -123[a:b]isn't subscriptable either. So the fix normalizes any non-Textnode tostr(node)once, up front (mirroring exactly whatrender()already does), and both the size computation and the slicing branch fall out correctly from that single normalization.Closes #305
Тип изменения
Как это было протестировано?
Added 5 regression tests in
tests/maxo/utils/test_formatting.py::TestGetItemNonTextNode: a slice spanning across an int node into a following str node, a slice landing entirely inside the stringified int node, a slice starting after the int node, a float node (to confirm this isn't int-specific), and the full-slice fast path (node[:]), which must leave_bodyuntouched (not pre-stringified).Confirmed the new tests fail against the pre-fix code (
TypeError: object of type 'float' has no len()) and pass after the fix, so they're real regression tests, not tautologies.Ran locally:
uv run pytest tests/ --cov=src --cov-report=term-> 1840 passed,src/maxo/utils/formatting.pyat 100% coverageuv run ruff check --no-fix .-> cleanuv run black --check .-> cleanuv run mypy --config-file pyproject.toml-> cleanuv run slotscheck -m maxo-> All OKuv run codespell src examples-> cleanТестовая конфигурация:
Контрольный список:
Summary by CodeRabbit