GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||
GeezerVison is showing github.com — gh-131878: Fix input of unicode characters with two or more code points in new pyrepl on Windows by sergey-miryanov · Pull Request #131901 · python/cpython Conversation
|
|
LGTM. I cannot do a thorough review, but at least confirm, that with this PR I can enter |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Tested with a Czech keyboard and everything works. Thanks!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Tomas R. <[email protected]>
…epl-for-long-unicode-chars
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Had some time now: this needs fixes for Linux. Otherwise looks good, thanks @sergey-miryanov! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Since this wasn't caught by the tests, we should also probably add a test for that. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I have suggested the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@chris-eibl Please take a look! I would like to keep tests for paste mode to be sure that escape sequences not mixed with input if error occurred. But I'm OK to remove them if you think it is not necessary. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
LGTM. Sure, more tests are always better. Though, AFAIU, there is nothing special about paste mode here:
IMHO, both are reasonable, but "paste mode" makes them look too special about pasting? So I'd rename them and use arbitrary "one byte chars" in there. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sequence of "one byte chars" is a control command to enable "paste mode". Originally in gh-131878 problem aroise from case where the wrongly passed unicode string puts to the buffer and mixed with following control command that disables paste mode and asserted in those code path. So, I want to check that we don't "corrupt" chars buffer. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yupp, I know. But as said, there is nothing special about bracketed pasting here. From the perspective of the eventqueue test, these are just arbitrary bytes. Whether bracketed pasting is working correctly, is tested in cpython/Lib/test/test_pyrepl/test_pyrepl.py Line 1201 in 1b7470f
If you'd like to showcase that sequences of "one byte chars" interleaved with "multi byte chars" work correctly via "paste mode", then maybe just add a comment? And small nit: cpython/Lib/test/test_pyrepl/test_pyrepl.py Lines 1227 to 1228 in 1b7470f
To me, those two tests have merit, but are too special about "paste mode". |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@chris-eibl It seems that I did not correctly assume how paste-mode works (shame on me!). I removed one test because the same code path is covered by |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I don't have a better name - the comment in there clarifies what we intend to test (not much tbh). Otherwise LGTM. Thanks for bearing with me! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Chris Eibl <[email protected]>
|
@chris-eibl Thanks for the review and patience! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thank you @sergey-miryanov!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Tested again after the simplifications. WFM on both Windows and Linux :)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Tomas R. <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Gentlemen, excellent work. Not only does it fix the bug, but it makes the codebase simpler and more elegant. If not for the new test, the net number of lines added here would be negative. Impressive!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks @sergey-miryanov for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry, @sergey-miryanov and @ambv, I could not cleanly backport this to |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@ambv Please take a look - this is the same problem with backport as in earlier PR - #130805 (comment) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…ore code points in new pyrepl on Windows (pythongh-131901) (cherry picked from commit 0c5151b) Co-authored-by: Sergey Miryanov <[email protected]> Co-authored-by: Tomas R. <[email protected]> Co-authored-by: Chris Eibl <[email protected]>
|
GH-133468 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…de points in new pyrepl on Windows (gh-131901) (gh-133468) (cherry picked from commit 0c5151b) Co-authored-by: Sergey Miryanov <[email protected]> Co-authored-by: Tomas R. <[email protected]> Co-authored-by: Chris Eibl <[email protected]>
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…e points in new pyrepl on Windows (pythongh-131901) Co-authored-by: Tomas R. <[email protected]> Co-authored-by: Chris Eibl <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.