hanschen / vim-ipython-cell

Seamlessly run Python code in IPython from Vim
GNU General Public License v3.0
335 stars 18 forks source link

Allow configuration of 'restart' function #39

Closed lukoshkin closed 2 years ago

lukoshkin commented 2 years ago

Hi! I've encountered a problem with your plugin that IPythonCellRestart just terminates IPython session instead of restarting it. This is because executing the previous shell command with Ctrl-P doesn't work for all shells. For me, it might be not working due to using Vi mode instead of Emacs in the shell cmdline.

IMO, it is a common practice that users customize their shells and Jupyter Notebook configurations, but do not change settings of IPython, a console app. If there is no a general solution (that accounts for different shells), it may be a good decision to allow a user to specify the command they use to get the previous shell command.

NOTE: I did not align the markdown table I edited in README, since it would lead to displaying meaningless changes in git diff.

hanschen commented 2 years ago

Thanks for the pull request. Overall the changes look good to me. I actually think '!!' as the default makes more sense than Ctrl-P - what do you think? If you agree, I would suggest the following:

  1. Change default for g:ipython_cell_shell_prev_cmd (and shell_prev_cmd parameter in restart_ipython) to '!!'.
  2. Replace '<ctrl-p> (case-insensitive) in shell_prev_cmd with the CTRL_P variable.
  3. Mention in the documentation that you can specify any value for g:ipython_cell_shell_prev_cmd, and that the special value <ctrl-p> can be used for Ctrl-P.
lukoshkin commented 2 years ago
  1. I agree, '!!' is a better default
  2. if shell_prev_cmd.upper() in ('<C-P>', '<CTRL-P>'): (Python code)- is that what you meant? It is case-insensitive for variations of <C-p>/<Ctrl-p> input strings
  3. I will update it tomorrow
hanschen commented 2 years ago
  1. if shell_prev_cmd.upper() in ('<C-P>', '<CTRL-P>'): (Python code)- is that what you meant? It is case-insensitive for variations of <C-p>/<Ctrl-p> input strings

I was thinking that <Ctrl-P>, <CTRL-P>, <ctrl-p> etc. (and could also be <C-p> etc.) could be replaced with the ANSI code. This would allow the user to also specify for example i<Ctrl-P> if they for example use vi mode. I guess this would require the use of regex for the substitution (don't think string.replace supports this kind of case-insensitive replacement).

In other words, g:ipython_cell_shell_prev_cmd would let the user specify any kind of command to run to restart IPython, with <ctrl-p>/<c-p> being a special value that is replaced with Ctrl-P.

lukoshkin commented 2 years ago

Hey, @hanschen! I've made some updates following your suggestions. Let's see what we've got

hanschen commented 2 years ago

Awesome, many thanks for your contribution!