Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork252
Conversation
thomasballinger left a comment
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 this seems like a safe choice, bpython doesn't seem to use Ctrl-g for anything and don't know if you checked something likehttps://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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 thatnew_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 theself.reevaluate(insert_into_history=True) inclear_modules_and_reevaluate() should probably be False instead, we can fix in another PR.
thomasballinger commentedJun 25, 2020
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you remove this extraneous change?
thomasballinger commentedJun 26, 2020
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 ind6bd2e5
|
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!