-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Handle lone surrogate unicode character not being representable in Jython #5271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle lone surrogate unicode character not being representable in Jython #5271
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov Report
@@ Coverage Diff @@
## master #5271 +/- ##
==========================================
+ Coverage 93.44% 93.95% +0.51%
==========================================
Files 115 115
Lines 26163 26165 +2
Branches 2578 2578
==========================================
+ Hits 24447 24583 +136
+ Misses 1395 1259 -136
- Partials 321 323 +2
Continue to review full report at Codecov.
|
f4373d1
to
8d98152
Compare
…thon No tests for this because we don't test with Jython currently. Fix pytest-dev#5256
8d98152
to
e253029
Compare
@asottile |
@blueyed I think it is fine, we can always do a follow-up. 👍 And after giving this some more thought, we needed this fix because our test suite used 😄 emoji in the messages (my suggestion), so it might not even happen in the wild as I doubt people are putting emojis in exception messages. Either way I think a follow up later (including a CHANGELOG adjustment) is fine. 👍 |
Sure. But there was also no reason to get this intermediate fix in fast - since we're not testing against Jython. Anyway, it was just a remark - nothing too bad. |
The current change is already ok for me (Jython works)... As a note, what I came up with in the end is below (where
I'm also a bit wary of |
You are right @fabioz, that could lead to problems. I propose we wait a bit though, I'm not sure this will really be a problem in the wild. If it does we can always fix it with an actual problem as guide. 👍 Thanks everyone! |
No tests for this because we don't test with Jython currently.
Fix #5256