Skip to content
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

Suggestion for a better python support #274

Merged
merged 16 commits into from
Apr 19, 2020
Merged

Conversation

incoggnito
Copy link
Contributor

@incoggnito incoggnito commented Apr 12, 2020

I've tried to optimize the python support depending on this issues:
#273
#270
Related:
#74, #89, #114, #233, #234

Branch demo:
1

I've added:

  1. python virtual environment
  2. clear the screen after REPL init --> TODO: hide till cleared
  3. interpreter run script (without pasting) --> TODO: add to other languages
  4. cell-mode behavior --> TODO: not tested except python
  5. paste magic for ipython

My settings:

let g:neoterm_default_mod = 'vertical'

# start python within a virtual environment like conda
let g:neoterm_repl_python_venv = 'conda activate py3conda'

# set the repl
let g:neoterm_repl_python = 'ipython'

# clear any startup screen like neofetch
let g:neoterm_clean_startup = 1

# use paste magic instead of the exec command
let g:neoterm_repl_ipython_magic = 1

# define a cell marker
let g:neoterm_repl_cellmarker = '^# %%'

README.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@incoggnito incoggnito mentioned this pull request Apr 13, 2020
autoload/neoterm/repl.vim Outdated Show resolved Hide resolved
autoload/neoterm/repl.vim Outdated Show resolved Hide resolved
autoload/neoterm/repl.vim Outdated Show resolved Hide resolved
autoload/neoterm/repl.vim Outdated Show resolved Hide resolved
autoload/neoterm/repl.vim Show resolved Hide resolved
Copy link
Owner

@kassio kassio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the proposal.
I would like to avoid creating more ways to send text to REPL instances, also, the goal o the PR is to fix the python REPL integration, so new ways of sending text to the REPL should be in another proposal. I would also rather to have as little options as possible, so let's try to fix that without adding more options to the code.

I did a few suggestions there, let me know what you think.

@incoggnito
Copy link
Contributor Author

incoggnito commented Apr 13, 2020

Thanks for the proposal.
I would like to avoid creating more ways to send text to REPL instances, also, the goal o the PR is to fix the python REPL integration, so new ways of sending text to the REPL should be in another proposal. I would also rather to have as little options as possible, so let's try to fix that without adding more options to the code.

I did a few suggestions there, let me know what you think.

Thank's for your explanation. I already suspected that it was a bit too much for a single pull request. Can absolutely understand your point of view regarding maintainability. In some places it may make more sense to simply expand the documentation instead of new functions. I like the idea with filetype specific script. I could also imagine to write and maintain a python-plugin based on neoterm myself. Would have some more ideas such as interactive debugging.

@kassio
Copy link
Owner

kassio commented Apr 13, 2020

I could also imagine to write and maintain a python-plugin based on neoterm myself.

That's an option, but I think with the filetype-specific REPL code most of the default behavior could be easily done. I'm looking forward to see this PR updated. 😄

@incoggnito
Copy link
Contributor Author

Everything works for python and ipython like in my first screen video. In addition, I have embedded the function of jkroes @ d6c5098. A python specific file was added.
That's my current settings:

let g:neoterm_default_mod = 'vertical'
let g:neoterm_repl_python = 'conda activate py3conda, clear, ipython'
let g:neoterm_repl_ipy_magic = 0
let g:neoterm_repl_python_cellmarker = '# %%'

I will continue to test this on occasion, but so far it seems to be working quite well.

autoload/neoterm/repl.vim Outdated Show resolved Hide resolved
autoload/neoterm/repl.vim Outdated Show resolved Hide resolved
autoload/neoterm/repl.vim Outdated Show resolved Hide resolved
@@ -0,0 +1,57 @@
"init python specific settings
if !exists('g:neoterm_repl_ipy_magic')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it magic a term used in the python community? Is the name of this variable is meaningful to someone starting in the python community?

Copy link
Contributor Author

@incoggnito incoggnito Apr 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is. Read here. The currently used command use is the ipython-paste-magic. I should probably insert the full name.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! That's good to know, thanks for sharing the reference.
So, I think we should the full qualified name here, just to be more clear:

if !exists('g:neoterm_repl_enable_ipython_paste_magic')

It's better to be a long name that it's more clear than a short one.

ftdetect/set_repl_cmd.vim Outdated Show resolved Hide resolved
@incoggnito
Copy link
Contributor Author

incoggnito commented Apr 17, 2020

I've been thinking for a while how to cover cell mode with the existing commands such as the g-x movement. Generally it's possible to search for the marker /# %% and use wyn to select the area and then send it. But for that you would have to put a marker at the beginning and end of the file. This is not a nice solution. What do you think?

I have already considered using the sign column to set graphical markers instead of the text markers. I*ve currently read the docs to understand how sign_define () and sign_jump () works.
That's would be a nicer and more intuitive neovim solution.

@@ -24,12 +24,11 @@ function! neoterm#repl#term(id)
if has_key(g:neoterm.instances, a:id)
let g:neoterm.repl.instance_id = a:id
let g:neoterm.repl.loaded = 1

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's keep the diff with the minimum. Avoid unnecessary changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sry.

ftdetect/set_repl_cmd.vim Outdated Show resolved Hide resolved
Comment on lines 11 to 19
if a:value[i] =~ 'python'
let l:pyCmd = 1
if a:value[i] == 'ipython'
let g:neoterm_repl_python[i] = 'ipython --no-autoindent'
endif
endif
if executable(split(a:value[i])[0]) == 0
let l:invalid = 1
endif
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In python i would create a function to handle the list item and the string. Do you know if it is possible here?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't understand your intention. What do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has already cleared up.

endif
endfunction

function! neoterm#repl#python#exec(command)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think to make things a bit more explicit in here, as well, like:

function! neoterm#repl#python#exec(command)
  if g:neoterm_repl_ipy_magic == 1 && join(g:neoterm_repl_command) =~ "ipython"
    let l:cmd = s:ipython_magic_command_for(a:command)
  elseif join(g:neoterm_repl_command) !~ 'ipython'
    let l:cmd = s:ipython_command_for(a:command)
  else
    let l:cmd = s:python_command_for(a:command)
  end

  call g:neoterm.repl.instance().exec(add(l:cmd, g:neoterm_eof))
endfunction

This way we'd have one private function for each way neoterm can send python commands to the REPL, it'd be easier to debug and easier to maintain. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, of course.
How do you debug vim scripts?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When needed, yes, but usually only with echo 🙈

doc/neoterm.txt Outdated
Comment on lines 234 to 235
Both a string and a list can be passed to. Defaults to an empty string,
in which case NeoTerm will fall back to IPython followed by Python.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Both a string and a list can be passed to. Defaults to an empty string,
in which case NeoTerm will fall back to IPython followed by Python.
This option accepts either a String or a list of commands. Its default
value is an empty string, which will try to use `ipython` or `python`,
in this order, to execute the REPL.

doc/neoterm.txt Outdated
Default value: (empty)

Example for a valid command list:
|g:neoterm_repl_python| = `['conda activate venv', 'clear', 'ipython']``
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's write the example in a way that users can copy'n'paste:

Suggested change
|g:neoterm_repl_python| = `['conda activate venv', 'clear', 'ipython']``
`let g:neoterm_repl = ['conda activate venv', 'clear', 'ipython']`

Comment on lines 47 to 48
return get(l:, 'ipycommand', pycommand)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return get(l:, 'ipycommand', pycommand)
return get(l:, 'ipycommand', pycommand)

end
endfunction

function! s:ipython_magic_command_for(command)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you put the private functions after the public function that call them? In this case, after the neoterm#repl#python#exec

Copy link
Owner

@kassio kassio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a changelog as well, please?

@kassio
Copy link
Owner

kassio commented Apr 18, 2020

can you also remove that sample.gif file, please?

@incoggnito incoggnito requested a review from kassio April 19, 2020 08:41
Copy link
Owner

@kassio kassio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the great contribution! ❤️💛💚💜💙🖤🧡

@kassio kassio merged commit 8a3a0fb into kassio:master Apr 19, 2020
@incoggnito
Copy link
Contributor Author

incoggnito commented Apr 19, 2020

Thank you for your support.
A great way to find out more about vim and open source.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants