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.

Sign up for GitHub

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

Redefine compile/debug commands to more sensible defaults (Fix 1693) #1697

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Redefine compile/debug commands to more sensible defaults (Fix 1693) #1697

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions elpy.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2657,10 +2657,20 @@ If you need your modeline, you can set the variable `elpy-remove-modeline-lighte
;; http://debbugs.gnu.org/db/13/13642.html
(set (make-local-variable 'forward-sexp-function) nil)
;; PEP8 recommends two spaces in front of inline comments.
(set (make-local-variable 'comment-inline-offset) 2))
(set (make-local-variable 'comment-inline-offset) 2)
;; Few if any python projects use Makefile, better let the compile
;; run the current script
(set (make-local-variable 'compile-command)
(if buffer-file-name
(concat "python " (shell-quote-argument buffer-file-name))
"python "))
;; Python debugger should be invoked as a module script
(set (make-local-variable 'gud-pdb-command-name) "python -m pdb"))
Comment on lines +2665 to +2668
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to use python-shell-interpreter instead of python, in case users customise it.

Copy link
Author

Choose a reason for hiding this comment

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

The problem I see with this is that people might have set ipython as their interactive interpreter. The request has been made in the past to allow for a different command than python-shell-interpreter to be set for debugging or testing, I can't remember which one right now.

At that time, one suggestion (I think it was yours, indeed) was to use elpy-rpc-python-command. It's not a very intuitive name for "non-interactive python command" but it's closer to what's needed here than python-shell-interpreter.

If you prefer I could open another PR with a new elpy-python-command option and patch every place where it's more sensible to use that than python-shell-interpreter. But I'm ok with using elpy-rpc-python-command anyway.

Alternatively (and much easier!), what do you think of providing a variable alias elpy-python-command -> elpy-rpc-python-command. Or maybe it's time to deprecate elpy-rpc-python-command in favor of the more general elpy-python-command, always understanding that this command is for non-interactive usage including launching the rpc server.

Copy link
Collaborator

@galaunay galaunay Oct 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Apparently (surprisingly) doing M-x compile RET ipython script.py works quite well.
But you are right on the substance (and it doesn't work with Jupyter...).

I am trying to add new options only when it is not absolutely necessary (there is already many of them).
So my opinion is to leave it to "python".
It sounds like a good compromise: It is a sane default and it is displayed in plain sight, so users can still modify it before execution if they don't like it.

Copy link
Author

@memeplex memeplex Oct 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Apparently (surprisingly) doing M-x compile RET ipython script.py works quite well.

It's ok to use ipython but then people configure their interactive shell to pre-import all sorts of heavy stuff like numpy and pandas and you have painfully slow debug sessions and unit test runs. And, of course, also jupyter console, as you say.

So my opinion is to leave it to "python"

So do you want me to change anything then?

It sounds like a good compromise

Sadly it's still problematic since many linux distros symlink python to python2. It's ok when you're in a venv though. But even with arch having done the migration a long time ago, fedora 31 doing it as we speak, there is still the entire debian/ubuntu branch of distros. Debian is slow moving and I seriously doubt that ubuntu will make the transition before the 20.04 LTS.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know it is not ideal, but the only other option is to add a new customisation variable.

I am not a big fan of the idea, as I find the current number of customisation variables already quite confusing (a lot of bug reports come from misconfigured variables, despite the doc).
So I would like to avoid adding new ones, if it is not really important.

In this case, users can still modify compile-command and gud-pdb-command-name by hand if necessary.
So IMO, having the right python version for those commands is not gonna be an important improvement.

Copy link
Author

Choose a reason for hiding this comment

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

In this case, users can still modify compile-command and gud-pdb-command-name by hand if necessary.

So we're adding config variables after all ;). But I think defaulting to elpy-rpc-python-command is a good bet, at least we know it's a working shell, while python would run python 2 in many cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to say "re-using them" :).
Let's go with using elpy-rpc-python-command, it will be "python" anyway if users don't bother to change it.

(`buffer-stop
(kill-local-variable 'forward-sexp-function)
(kill-local-variable 'comment-inline-offset))))
(kill-local-variable 'comment-inline-offset)
(kill-local-variable 'compile-command)
(kill-local-variable 'gud-pdb-command-name))))

;;;;;;;;;;;;;;;;;;;
;;; Module: Company
Expand Down