-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-128067: Fix pyrepl overriding printed output without newlines #138732
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
base: main
Are you sure you want to change the base?
Conversation
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
As written in the issue, this works fine on
|
| self._move_relative(0, len(self.screen) - 1) | ||
| self.__write("\n") | ||
| if self.screen: | ||
| self._move_relative(0, len(self.screen) - 1) |
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.
Spot on, len(self.screen) - 1 should not get negative here.
|
Please add a News entry, see Quick reference #8 in the devguide. |
|
Please merge with latest default, because the pyrepl Windows tests are not run otherwise (#138801). |
So merge with current main and "merge" not "rebase", right? So basically just clicking the "Update branch" button here. |
|
I suggest to add tests like def test_no_newline(self, _os_write):
code = "1"
events = code_to_events(code)
_, con = handle_events_unix_console(events)
self.assertNotIn(call(ANY, b'\n'), _os_write.mock_calls)
con.restore()
def test_newline(self, _os_write):
code = "\n"
events = code_to_events(code)
_, con = handle_events_unix_console(events)
_os_write.assert_any_call(ANY, b"\n")
con.restore()in I think they are best placed before Both tests fail before the PR and succeed with the PR. So these tests might be enough, but of course your current test is more explict. But since Windows doesn't have the pty (Pseudo-terminal utilities), I don't know how to do a similar test there. The filtering of the many escape sequences might induce some maintenance burden, but I don't think it is a big issue. |
Correct. For reviewers it is easier to follow without squashing. For the final merge from your fork to the upstream branch, squashing is used, anyway. |
|
Added the tests. I put the news entry under Windows because the main user visible effect here is on windows, although we did make changes to both unix and windows consoles. |
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.
LGTM. Thank you @JanEricNitschke.
|
I just found out, that in cpython/Lib/_pyrepl/windows_console.py Line 517 in 8b5ce31
to cpython/Lib/_pyrepl/unix_console.py Line 562 in 1c984ba
like in unix_console.py (and like it is initialized in prepare in both implementations).
|
|
Changed this for the windows console to be the same as for the unix one. |
|
Added myself to ACKS after the review/approval, hope thats alright. |
I tried a bit to debug this and narrowed it down to these two lines for windows and unix terminals respectively:
Because while properly running the repl in unix doesnt cause the issue, running the test i added also fails there before these changes.
I am not 100% sure if this is the base and 100% correct way, but it causes the test to pass on unix and fixes the issue on windows for me.
I also attempted to try to be able to run these tests for windows (because thats where we actually saw the issue) but didnt manage unfortunately.
With the fix in the unix console:
Without: