-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
gh-119034, REPL: Change page up/down keys to search in history #123607
Conversation
Change <page up> and <page down> keys of the Python REPL to history search forward/backward.
Example 1: Search history with
Example 2: Navigate history.
|
Thanks! This feature would make the new REPL usable for me. |
This behavior is consistent with the old REPL. I verified it works.
As said by @encukou, this behavior is not consistent with the old REPL. I have verified it indeed behaves as you describe, which is not ideal (but still better than not having this at all). |
I don't know pyrepl code base, if you have advices to make these commands (page up/down) closer to readline, i'm interested. |
How so? I mean, you can use Ctrl-r and similar to go to specific entries no? |
No. See my comment from #119034 (comment) This particular readline feature is about PageUp and PageDown, not about arrows up and down. What this proposes is to change the behavior of PageUp and PageDown in line with what was established in #119034 (comment) What #121859 / #120767 proposes is to change the behavior of arrow up/down which was not established in #119034 (comment)
That's a different functionality of the REPL. This allows us to write partial text first and press PageUp after. Ctrl+r requires us to use it before we start to type. The PageUp/PageDown functionality has been the default in Fedora readline for 15 years and not having it is the reason for me not to use the new REPL. |
But isn't that the same feature with different bindings? What am I missing? |
I understand and respect that but I am a bit surprised that the lack of this outweights multi-line editing and paste mode, among other new features. So I am trying to understand why is this so fundamental. |
I updated my PR to behave as readline. I managed to find a way to implement the expected behavior. |
The apparent consensus of #119034 (comment) It is the same (similar?) feature, but binding it to different keys (which already have different functionality) is against that consensus.
The new REPL has many nice improvements and I am looking forward to using it, yet it fails to behave the way I expect a REPL to behave. I reported that problem and I was told yes, this is a problem, we are planning to fix this, but instead, a different fix was proposed. Imagine I give you a new bicycle with perpetual motion automatic power, a super comfy seat, and it can be folded into a matchbox-size box, so you can carry it around in your pocket. Yet it does not allow you to steer. The lack of steering capabilities outweighs all the perks. For me, the basic functionality of REPL outweighs all the fancy new stuff. I open the REPL, I type |
Thank you. It work almost as readline now, except I cannot PageDown back to empty prompt. Old REPL>>> [page up]
>>> whatever[page down]
>>> New REPL with this PR>>> [page up]
>>> whatever[page down]
>>> whatever |
Right, the behavior looks very similar: match the beginning of the line to navigate history. I tested gh-121859 : there are subtle but major differences. The main one is that in my PR (page up/down), the cursor doesn't move when matching the prefix, whereas it moves at the end with gh-121859. The usability/behavior is different. Example:
Using <page up> and my PR, you only get
Using <up> and gh-121859, the first <up> looks for
I suggest you to play with both PR to see/understand the subtle differences. This PR is navigate all entries starting with |
I fixed the last PageDown to be able to go back to an empty prompt. |
There seem to be some kind of state leak, see ( >>> imp|[page up]
>>> imp|ort os
>>> imp|[Ctrl+k]ort os
>>> imp|[page up]
>>> imp|ort sys
>>> imp|[page down]ort sys
>>> imp|[page down]
>>> While in the old REPL: >>> imp|[page up]
>>> imp|ort os
>>> imp|[Ctrl+k]ort os
>>> imp|[page up]
>>> imp|ort os
>>> imp|[page down]ort os
>>> imp|[page down]ort os
>>> imp|[page up]ort os
>>> imp|ort sys |
Just to be clear, what I am a bit concerned is that the contributor in #121859 may feel that we ignored her PR and implemented most of the required changes separately, leaving the original issue just as a rebinding or (maybe?) closed because the lack of consensus. |
Ok, I understand your point of view, but I personally would not elevate this particular feature as a "basic functionality". The basic functionality of the REPL is to execute what you type, anything else it's additional (nice to have) features. This particular feature we are talking about was even a readline functionality that's only available if you have configured the But in any case, I understand your point and it makes sense. Let's fix it so is as close as possible to the old REPL and the bycicle can steer again. But @vstinner @encukou please take #123607 (comment) into account here |
It has been enabled by default for all the Fedora Linux, RHEL and CentOS users for 15 years. So from my PoV it might as well have been "built into" the old REPL. I agree that if all other functionality of REPL beyond "input code and output the result" is additional, then this is as well. By "basic" I've meant "important enough to stop me from using it". |
I don't think that the partial history on arrows feature is contradicticting this one. I have not seen it in any REPL (and my opinion whether Python REPL needs it is not relevant), but whether or not it is added does not need to block the PageUp/Down feature. |
It's not the feature: it's the fact that giving that the code will overlap, merging this PR first for example may eliminate the need for a lot of code in the other one and leave it just as rebinding, which may be a bad experience for the contributor. That's what I am worried about |
I understand 👍
Gotcha |
This is mostly good. It currently doesn't support searching within subsequent lines of multiline history items, which I think it should be doing. @hroncok should the PgUp/PgDn search ignore leading whitespace or should it require leading whitespace in this scenario? |
See my comment in #123607 (comment) -- I think this is not good.
Good point, I was not testing that.
It does not do that normally.
I don't understand. What do you mean? |
If somone wants to test readline's built-in commands on Linux other than Fedora, the following
|
…ythonGH-123607) Change <page up> and <page down> keys of the Python REPL to history search forward/backward. (cherry picked from commit 8311b11) Co-authored-by: Victor Stinner <vstinner@python.org> Co-authored-by: Łukasz Langa <lukasz@langa.pl>
GH-123773 is a backport of this pull request to the 3.13 branch. |
Thak you, I'll check the new behavior out, but perhaps not in time for rc2 |
I looked at this scenario, but I failed to mimick readline's exact behavior in pyrepl which works on the temporary history while the history is being modified. |
…GH-123607) (GH-123773) Change <page up> and <page down> keys of the Python REPL to history search forward/backward. (cherry picked from commit 8311b11) Co-authored-by: Victor Stinner <vstinner@python.org> Co-authored-by: Łukasz Langa <lukasz@langa.pl>
Change page up and page down keys of the Python REPL to history search forward/backward.