Repository navigation
Strip U+0080 instead of crashing change_to_ascii - #1017
arpitjain099 wants to merge 2 commits into
Conversation
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
|
This PR is not associated with any issue, please create one first to describe this bug. In addition, please use our PR template. |
|
Both done: opened #1018 with the reproducer and the stack trace, and rewritten the description against the template with the LLM disclosure filled in. Two things I left blank rather than guess at. I did not touch |
This does need to be included in the changelog. If you want you could add this under You could also add the |
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
|
Added, as a That also clears |
Pull Request Checklist for MontePy
Description
ASCII ends at 0x7F, but the character by character fallback in
strip_characterstestsord(char) > 128, so U+0080 is kept. That branch only runs once the whole line has already failed to encode, and the join is re-encoded witherrors="strict", so the kept byte raisesUnicodeEncodeErrorout of the script. A single U+0080 therefore kills the tool whose job is to remove exactly that.Walking the boundary through
main: a pure ASCII file is copied unchanged, U+00E9 (ord 233) is stripped correctly, and U+0080 raises.The expected value helper in
tests/test_scripts.pymirrored the same boundary with<= 128in three places, so it agreed with the bug and could never catch it. I moved those to<= 127as well, otherwise the test would push the next person back toward the old comparison.Fixes #1018
General Checklist
blackversion 25 or 26.LLM Disclosure
Are you?
The account belongs to Arpit Jain, who directs the work and reviews it before it is submitted. This change was prepared by an LLM on his behalf, and this description was written by it, so the second box is the accurate one.
Were any large language models (LLM or "AI") used in to generate any of this code?
claude-opus-5)Each commit also carries a
Co-Authored-By: Claude <noreply@anthropic.com>trailer.Documentation Checklist
.. versionchanged::or.. versionadded::directives.The first two are vacuous here: no class, method or function is added, the source change is one character in an existing comparison. The third is unticked because no documented API changes behaviour, so there is nothing for a
versionchangeddirective to attach to. The release note lives in the changelog instead, added as a#Next Version#section per your comment below.Infrastructure Changes
Not an infrastructure change.
Significant features or Behavior changes
Not a new feature.
First-Time Contributor Checklist
pyproject.tomlif you wish to do so.Skipping this one.
Additional Notes for Reviewers
Ensure that:
Left unticked because they read as yours to confirm rather than mine to assert. For the last one, both branches of the changed comparison are exercised: U+0080 takes the replace path and U+00E9 already did, in both the
-dand-wvariants.The two new cases fail on
developwithUnicodeEncodeError: 'ascii' codec can't encode character '\x80' in position 4and pass with the change.tests/test_scripts.pygoes from 2 to 4 passing, and the full suite is 1250 passing.One thing worth flagging separately:
test_num_collect_clonehas a 200ms Hypothesis deadline and tripped once in three full runs for me, measuring 321ms then 88ms on replay. It is unrelated to this change and passes on re-run, but the margin looks thin on a slow machine.📚 Documentation preview 📚: https://montepy--1017.org.readthedocs.build/en/1017/
📚 Documentation preview 📚: https://montepy--1017.org.readthedocs.build/en/1017/