Conversation
thomasballinger
left a comment
thomasballinger
left a comment
There was a problem hiding this comment.
This approach seems solid, so I went ahead with comments about the nitty-gritty.
Trying it out, I like it. I often undo several lines, then hit up arrow multiple times to get back the old value to redo that line, so this is an improvement.
bpython/curtsiesfrontend/repl.py
Outdated
| self.on_tab(back=True) | ||
| elif e in key_dispatch[self.config.undo_key]: # ctrl-r for undo | ||
| self.prompt_undo() | ||
| elif e in ["<Ctrl-g>"]: # for redo |
There was a problem hiding this comment.
👍 this seems like a safe choice, bpython doesn't seem to use Ctrl-g for anything and don't know if you checked something like https://en.wikipedia.org/wiki/GNU_Readline but it doesn't sound like something people are clamoring for. It would be nice to make it configurable like self.config.undo_key.
There was a problem hiding this comment.
Also it'd be nice to
- follow the pattern of other keys of calling a method to keep this switch-like if/elif/elif/... structure shorter
- also for symmetry, use a tuple here instead of a list
bpython/curtsiesfrontend/repl.py
Outdated
| elif e in key_dispatch[self.config.undo_key]: # ctrl-r for undo | ||
| self.prompt_undo() | ||
| elif e in ["<Ctrl-g>"]: # for redo | ||
| if (self.could_be_redone): |
There was a problem hiding this comment.
I know we chose this together but I don't like it anymore, it sounds to me like a boolean. How about redo_stack?
bpython/curtsiesfrontend/repl.py
Outdated
| self.on_tab(back=True) | ||
| elif e in key_dispatch[self.config.undo_key]: # ctrl-r for undo | ||
| self.prompt_undo() | ||
| elif e in ["<Ctrl-g>"]: # for redo |
There was a problem hiding this comment.
It'd be nice to follow the pattern of the other keys here, calling a method in the body of the if.
bpython/repl.py
Outdated
| ) | ||
| self.s_hist = [] | ||
| self.history = [] | ||
| self.history = [] # History of commands |
There was a problem hiding this comment.
how about "commands executed since beginning of session," this comment isn't helping me much now.
bpython/repl.py
Outdated
| entries = list(self.rl_history.entries) | ||
|
|
||
| #Most recently undone command | ||
| lastEntry = self.history[-n:] |
There was a problem hiding this comment.
Could you change lastEntry -> last_entries; plural because it's a list, and snake_case
bpython/curtsiesfrontend/repl.py
Outdated
|
|
||
| def on_enter(self, insert_into_history=True, reset_rl_history=True): | ||
| # so the cursor isn't touching a paren TODO: necessary? | ||
| if insert_into_history: |
There was a problem hiding this comment.
This if statement reads confusingly to me, I think we should:
- rename the
insert_into_historyparameter inRepl.reevaluate()tonew_code; this is what it means, it's about whether the lines might be new, so they should be added to readline history. - rename the
insert_into_historyparameter inRepl.on_enter()tonew_codeas well. Then this if becomes "if the line being executed is new, then we can no longer use our redo stack." - not rename insert_into_history anywhere else
My rationale for the name is that new_code describes the situation/circumstance instead of the behavior we want, which is important since we're now using this flag to determine a new, different behavior.
And in reviewing this I found that the self.reevaluate(insert_into_history=True) in clear_modules_and_reevaluate() should probably be False instead, we can fix in another PR.
Could do, I don't think it's a big improvement.
I'd move it to a method on this Repl class, maybe right below |
bpython/curtsiesfrontend/repl.py
Outdated
| greenlet.greenlet(prompt_for_undo).switch() | ||
|
|
||
| def reevaluate(self, insert_into_history=False): | ||
| def prompt_redo(self): |
There was a problem hiding this comment.
I think just redo, "prompt_undo" is because it may prompt you for how many lines you want to undo.
bpython/cli.py
Outdated
| self.undo(n=n) | ||
| return "" | ||
|
|
||
There was a problem hiding this comment.
Could you remove this extraneous change?
|
Ok one more thing! (I'd say on last thing but that's hard to promise) Could you add this to the example config file, as a commented out key like undo: Line 69 in d6bd2e5 |
2 participants
I think I fixed it! But it does feel a little "hack-y." You previously suggested we make history a class, do you still think that's a good idea? You also suggested we move redo somewhere else, though I don't know if you had ideas of where-- do you still have thoughts on that? Thanks!