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

Allow better code sending to shell #1232

Conversation

galaunay
Copy link
Collaborator

@galaunay galaunay commented Dec 4, 2017

Apparently Elpy users have a wide variety of workflows that necessitate different relations between python buffers and interactive shells (see PR #839 and issues #1100 and #1197).
This is an attempt to allow all those different workflows while keeping the code simple.

This PR defines a new option elpy-shell-code-sending-strategy which can be used to control to which python shell the code should be sent.

Available options are:

  • on-shell: the default behavior of elpy. everything is sent to a main python shell.
  • dedicated-shells: the old behavior of elpy when elpy-dedicated-shells was non-nil. Each buffer send its code to a dedicated python shell. Functionality introduced by PR Fix dedicated shells support  #839.
  • manual-switching: the code is sent to a main python shell for all buffers, but you can switch between different main python shells with elpy-shell-switch-python-shell. Functionality discussed in multiple python session #1197.
  • project-shell: one shell per project (as identified by elpy-project-root). Functionality discussed in Add project shell with project root in sys.path #1100.
  • ask: Ask to which buffer the code has to be sent for each buffer (only the first time code is sent).

After implementing and testing it, I am not sure the ask option is really useful...

One advantage of this implementation is that code modifications are restrained in elpy-shell-get-or-create-process.

I am waiting for some feedback on this before finalizing (notably updating the tests and the documentation).

@gopar
Copy link
Collaborator

gopar commented Dec 5, 2017

From the discussion in #1100 it looks like @jorgenschaefer was prefering to one single shell that has project root loaded in. He also mentioned that dedicated shells wasn't really ideal anymore but giving people the option is nice.

As a user theres only two things I want. (This is just my opinion though :P)

  1. A shell with project root loaded in (I would use this 90% of time)
  2. A stand alone shell independent from everything else to test out snippets of code.

I don't see the use cases for the other options (Also I think I agree with you that asking every time isn't ideal). I think less code is better code :P but eh, let jorgen decide \o/

@galaunay
Copy link
Collaborator Author

galaunay commented Dec 5, 2017

Well, the project-shell option should do what you want (on shell per project with project root loaded).

Personally, while doing data analysis, I often end up having repositories containing a bunch of scripts that I want to start independently, that's why I like the possibility to have dedicated shells.

You are absolutely right about code simplicity, I guess I will wait for more feedback to see if this really benefit the community.

@rgemulla
Copy link
Collaborator

rgemulla commented Dec 5, 2017

I fear that we are adding too much unneeded complexity here.

@gopar: do you use the send-to-shell-commmands to send statements from the Python buffers to the standalone shell? Or do you just use it to type stuff manually?

@gopar
Copy link
Collaborator

gopar commented Dec 5, 2017

@rgemulla I usually never select a region of code to send to a shell. Mostly because the snippets of code I want to check need to know about the project (where other modules are, etc).

Maybe we can only have one or two python shell commands and the rest be custom functions that the user can setup via looking up in the wiki or docs of Elpy? Just spitting ideas, since it will make it easier to maintain if theres less functions

@gopar
Copy link
Collaborator

gopar commented Dec 5, 2017

Also, @galaunay if you need to run python scripts independently, then you could either use M-x Compile or if you need to add input then run compile-mode to comint-mode. And that should be able to what you need it to do (But I don't know what your exact needs are so this might not be an appropriate solution)

@rgemulla
Copy link
Collaborator

rgemulla commented Dec 5, 2017

@gopar So the only functionality you need from Elpy are commands to start a shell for the current project (and afterwards you manage them yourself), right? Then this is the wrong PR ;) -- Anyway, I think we may add two commands, one that creates a shell for the current buffer and one that creates a new shell for the current project.

@galaunay The key question is whether we really need to associate a shell buffer with each input file or not. If so, I suggest minor changes: one option elpy-shell-default-shell for the default shell with values global, buffer, project, one function to switch the shell for just the current buffer (elpy-shell-set-local-shell or so), and one function to switch the global shell (elpy-shell-set-global-shell). It's probably a good idea to have a separate function for returning the current shell name for a given buffer (elpy-shell--get-shell-buffer-name), and one for creating a shell which calls this function.

@galaunay
Copy link
Collaborator Author

galaunay commented Dec 5, 2017

Well, if I am the only one to use dedicated shells, we should definitely get rid of it.

We can however add a note somewhere in the documentation to explain how to manage shells interaction:

  • dedicated shells behaviour can be achieved by locally setting python-shell-buffer-name.
  • switching 'session' (see multiple python session #1197), can be achieved by changing globally python-shell-buffer-name.

as I misunderstood @gopar PR, I don't think the project shell (as implemented here, i.e. shells dedicated to each projects) will be necessary once #1100 will be merged.

@rgemulla we can add elpy-shell-set-local-shell and elpy-shell-set-global-shell but they should be one or two liner functions (basically (setq-local python-shell-buffer-name new-name) and (setq python-shell-buffer-name new-name). So I wonder if it is better to implement them, or to explain the trick in the doc.

I will give it a try in the next days, to see where it goes.

@rgemulla
Copy link
Collaborator

rgemulla commented Dec 5, 2017

I am not sure if you are the only one using dedicated shells ;). And since you are doing the work, your workflow counts!

Anyway, the advantage of having these as commands is that one can bind them to keys / run them interactively and support auto completion. But I am additionally in favor of updating the docs, in particular, mentioning that one can also fix the shell using a buffer-local variable (so that one does not have to call elpy-shell-set-local-shell).

@jorgenschaefer What's your opinion? Drop dedicated shells + commands to switch the shell manually, or have more sophisticated functionality for setting the default shell to use?

@galaunay galaunay force-pushed the Allow_better_script_redirection_to_shell branch from 9dd6707 to 7809117 Compare December 7, 2017 11:52
@galaunay
Copy link
Collaborator Author

galaunay commented Dec 7, 2017

So, after thinking about the previous discussions:

  • I removed the complicated implementation of 'shell strategy'.
  • I removed the support for dedicated shells (@jorgenschaefer also wanted this, c.f Add project shell with project root in sys.path #1100)
  • I added elpy-shell-toggle-dedicated-shell to still allow to have dedicated shells, without interfering with elpy basic functions.
  • I added elpy-shell-change-global-shell-name to allow workflows similar to the one discussed in multiple python session #1197 (without interfering with Elpy basic functions).

I am concerned about the fact that elpy-dedicated-shells will be obsolete, what do you think will be the best way to inform users about this ?

If it sounds good for everyone, I still need to update tests and documentation.

@coveralls
Copy link

coveralls commented Dec 7, 2017

Coverage Status

Coverage remained the same at 89.332% when pulling 7809117 on galaunay:Allow_better_script_redirection_to_shell into 1f3a9a1 on jorgenschaefer:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.332% when pulling 7809117 on galaunay:Allow_better_script_redirection_to_shell into 1f3a9a1 on jorgenschaefer:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.332% when pulling 7809117 on galaunay:Allow_better_script_redirection_to_shell into 1f3a9a1 on jorgenschaefer:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.332% when pulling 7809117 on galaunay:Allow_better_script_redirection_to_shell into 1f3a9a1 on jorgenschaefer:master.

@galaunay galaunay mentioned this pull request Dec 7, 2017
@gopar
Copy link
Collaborator

gopar commented Dec 7, 2017

Just some thoughts.

Wouldn't it be easier to create 2 functions? One for project shell and the other for stand-alone/dedicated shells? That way the user knows whats happening. For example, right now elpy-shell-get-or-create-process will return a python shell but the user won't know if it's a dedicated shell or one that has project root in sys.path already. Just my two sense but a function should strive to return the same output no matter what the conditions are, but that one may return different output depending on the environment :P

If you have a function named elpy-switch-to-project-shell or something, we can explicitly document that this will load project root into python shell and will fail if there is not project root. (Like when running stand-alone scripts) Plus we can change buffer name to something like Proj[%s] to have it as a visual cue as well.

Also, for creating multiple dedicated shells, you might want to look into uniquify-buffer-name-style for help creating 'unique' buffer names instead of prompting for a name. (The user can always rename the buffer if they wish) Also, I just quickly glaced at that variable, but it might be of some help.

Not sure if these suggestion sound like a good idea. I just don't want to add unnecessary maintenance and potentially burn out any of us from contributing :P

@gopar
Copy link
Collaborator

gopar commented Dec 7, 2017

Also, please let me know if any of my comments sounds harsh. I would hate to sound mean in any way. Sorry if I did!

@rgemulla
Copy link
Collaborator

rgemulla commented Dec 7, 2017

@galaunay I like these changes (but haven't tested them yet). Instead of having toggle-dedicated-shell, why not have set-local-shell with auto completions that include (1) all running python shells, (2) a "global" option (which clears the buffer-local variable), and (3) the "Pyhton[%s]" option? Also, do the buffer names include the leading/trailing star (*Python* instead of Python)?

@gopar Some people (including me) often send commands from Python buffers to the corresponding shell. For us, it's not about starting shells, it's about which of the shells the commands get sent to. That being said, I guess one could have two different "start a shell" functions (one for here, one for in project root) plus a default one which chooses which one to take based on "elpy-use-project-root".

@galaunay
Copy link
Collaborator Author

galaunay commented Dec 7, 2017

@gopar with this implementation, the project root will always be on sys.path (dedicated shell or not).
I thought you agree with @jorgenschaefer in #1100 that project root on sys.path should be done by default, am I missing something ?

@gopar at this state, dedicated shell names are created using the name of the associated buffer (*Python[<buffer name>]*), which is already supposed to be unique. But this could change following @rgemulla propositions, so I take note of this uniquify-buffer-name-style function :).

@rgemulla This will definitely allows more modularity, I will make some tries to see if it is easily doable.
The shell buffer names do include leading/trailing stars.

@galaunay
Copy link
Collaborator Author

galaunay commented Dec 8, 2017

Okay, I implemented a version of elpy-shell-set-local-shell following the proposition of @gopar.
It is quite complex due to the gathering of existing shells and the prompt showing the current associated shell.
But this is definitely easier to use.

We will have to choose between elpy-shell-set-local-shell and elpy-shell-toggle-dedicated-shell at some point.

@coveralls
Copy link

coveralls commented Dec 8, 2017

Coverage Status

Coverage remained the same at 92.71% when pulling 330dd14 on galaunay:Allow_better_script_redirection_to_shell into 712804a on jorgenschaefer:master.

@rgemulla
Copy link
Collaborator

rgemulla commented Dec 8, 2017

This looks quite useful to me. I will play around with it as soon as I find some time and provide more feedback.

@ganesh-krishnan
Copy link

Any update on merging this branch? I'd love the functionality in this branch. 😃

@galaunay
Copy link
Collaborator Author

@ganesh-krishnan It's on my todo list.
But I admit I was a little repelled by the time it may need to make a proper PR from this.

Thanks for showing your interest :)
I will take a look, see if I can reach a closure for this.

@galaunay galaunay force-pushed the Allow_better_script_redirection_to_shell branch from 6fa8cc4 to df67ef4 Compare February 12, 2018 18:57
`elpy-shell-toggle-dedicated-shell` attaches a dedicated shell to the
current buffer.
`elpy-shell-set-local-shell` attaches a specific shell to the current
buffer.
@galaunay galaunay force-pushed the Allow_better_script_redirection_to_shell branch from df67ef4 to a725f76 Compare February 12, 2018 21:43
@galaunay
Copy link
Collaborator Author

galaunay commented Feb 12, 2018

Ok, to summarise a bit this meandering PR:

  • elpy-dedicated-shells option is removed, to simplify the central function elpy-shell-get-or-create-process.

  • Two functions are added to allow a more granular control on what python buffer should be attached to what python shell:

    • elpy-shell-toggle-dedicated-shell attaches a dedicated shell to the current python buffer.
    • elpy-shell-set-local-shell attaches a specific shell (asked with completion) to the current python buffer.

    This has the advantage of allowing any imaginable workflow (one shell per file, one file per project, manual switching between shells, ...) while keeping the code simple.

  • Documentation has been updated to explain how to use those functions.

@rgemulla @gopar @ganesh-krishnan any proposition or remarks on this are welcome !

nb: I will add the additional needed tests if we reach a final decision.

@gopar
Copy link
Collaborator

gopar commented Feb 13, 2018

Nice!

I gave a quick glance and looks good. 👍

@jorgenschaefer
Copy link
Owner

Not sure if my input is still needed here. I prefer simplicity and fewer options, so this looks great. :-)

@galaunay galaunay force-pushed the Allow_better_script_redirection_to_shell branch from c0c590b to 330dd14 Compare February 16, 2018 21:38
@galaunay
Copy link
Collaborator Author

Ok,
I just reviewed the documentation and added a bunch of tests.

Thanks for the feedbacks.

@galaunay galaunay merged commit d89cbca into jorgenschaefer:master Feb 16, 2018
@galaunay galaunay deleted the Allow_better_script_redirection_to_shell branch February 16, 2018 22:09
@ganesh-krishnan
Copy link

thanks @galaunay. much appreciated!

@yitang
Copy link

yitang commented Mar 22, 2020

@galaunay thanks. this is very useful for data sciences workflow.

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

Successfully merging this pull request may close these issues.

None yet

7 participants