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

Update dap-python for running in pipenv #293

Closed
wants to merge 5 commits into from

Conversation

MokkeMeguru
Copy link

We have many options for executing the python program.
At first, I want to add "pipenv" into the option.

We have many options for executing the python program.
At first, I want to add "pipenv" into the option.
dap-python.el Outdated Show resolved Hide resolved
dap-python.el Outdated Show resolved Hide resolved
dap-python.el Outdated
(python-executable (dap-python--pyenv-executable-find dap-python-executable))
(virtualenv-type (plist-get conf :virtualenv))
(python-executable
(cond ((= virtualenv-type "pyenv")
Copy link
Member

@ericdallo ericdallo Jun 8, 2020

Choose a reason for hiding this comment

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

I think string= is better for string comparison.
Is there any reason for virtualenv-type to be a string? a symbol could fit better, what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I afraid that symbol will pollute global variables... (I know emacs-lisp a bit.)
string= is better

Copy link
Author

Choose a reason for hiding this comment

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

If the symbol is correct in terms of emacs lisp, please tell me.

Copy link
Member

Choose a reason for hiding this comment

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

Symbols don't pollute the global namespace, unless you intern them, which you needn't do. A symbol ('foo) is just a lisp object like an int, string, .... You could use (eq virtualenv-type 'pyenv) instead of (= virtualenv-type "pyenv"), the latter invalid anyway since = can only compare ints and markers.

Co-authored-by: Eric Dallo <[email protected]>
@yyoncho
Copy link
Member

yyoncho commented Jun 8, 2020

@mpanarin, willing to take a look?

@mpanarin
Copy link
Contributor

mpanarin commented Jun 8, 2020

It is the same as #202
To be honest, I am against explicit support of the virtualenv in lsp-mode or dap-mode.
This should be handled by emacs and its executable-find. There are a bunch of packages and minor-modes that will make executable-find find a proper python in virtualenv. I mentioned a few in the issue above.

The problem with supporting virtualenvs is that everyone uses them differently. You can use them through pipenv, 'virtualenv', 'virtualenvwrapper', 'poetry' to name a few. And I don't think we should write support for all of them.
pyenv is a bit different as it is not really a virtualenv, so we have a direct support for it.

But I am definitely open for a discussion @ericdallo @seagle0128 @yyoncho

@ericdallo
Copy link
Member

I agree with @mpanarin, IMO we should make dap-mode extensible and easy to setup with different templates, support all possible virtualenvs seems to be too much.

@MokkeMeguru
Copy link
Author

But now you are supported pyenv. If you want to do so, you should remove pyenv option, shouldn't you?

@MokkeMeguru
Copy link
Author

MokkeMeguru commented Jun 8, 2020

In my opinion, we can give the execute-python-path into a template.
Can you fix it?

@mpanarin
Copy link
Contributor

mpanarin commented Jun 8, 2020

@MokkeMeguru pyenv is a version manager, not a virtualenv manager.
As I said, it is a different story, so we support it explicitly

MokkeMeguru added a commit to MokkeMeguru/dap-mode that referenced this pull request Jun 8, 2020
@MokkeMeguru
Copy link
Author

If so, then we must consider conda, and Pyflow... It will be too complex problem...

@greboide
Copy link

So how to run dap on python with pipenv? No way?

@htgoebel
Copy link
Contributor

@MokkeMeguru @mpanarin Since this pull-request is not accepted and #294 is an alternative: can this one be closed?

@MokkeMeguru
Copy link
Author

hi @htgoebel
IMO, this pr can be closed.
thanks for your review

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.

7 participants