Skip to content

Conversation

uoo723
Copy link
Contributor

@uoo723 uoo723 commented Dec 19, 2017

The common pattern of creating virtualenv is creating subdirectory.

$ virtualenv projectDIR/venv

So, I added code that searches that directory and assigns proper value for VIRTUAL_ENV.

@lgeiger
Copy link
Collaborator

lgeiger commented Dec 25, 2017

The common way of using ide-python is to add the project directory you want to work with to your Atom project in the tree view. This way venv would be on the top level.
Could you elaborate on your use case why this isn't possible?

@uoo723
Copy link
Contributor Author

uoo723 commented Dec 26, 2017

In my case, I usually put the venv in root project.
So, It looks like this.

myproject
+-- venv
|     +-- bin
|     +-- include
|     +-- lib
+-- django
|     +-- django
|     |     +-- __init__.py
|     |     +-- settings.py
|     |     +-- urls.py
|     +-- app
|     |     +-- __init__.py
|     |     +-- apps.py
|     +-- manage.py
+-- .gitignore
+-- README.md
+-- requirements.txt

In this case, ide-python doesn't seem to aware venv.

@lgeiger
Copy link
Collaborator

lgeiger commented Dec 26, 2017

@uoo723 Thanks for elaborating. I'm not familiar with virtual envs but I think this makes sense to add.
I'm not sure if it adds a lot to the startup time of the package but I guess it should be OK.

Copy link
Collaborator

@lgeiger lgeiger 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 PR 🎉

Could you update the code to use asynchronous methods as much as possible, so ide-python won't block the UI.

lib/main.js Outdated
var venvPath = "";

new Directory(projectPath)
.getEntriesSync()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you use the asynchronous method getEntries() because getEntriesSync() can block the UI if it takes too long?

Personally I'd wrap it in a Promise to be able to use it with async/await but you can also use native callbacks. What ever you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just updated.

lib/main.js Outdated
.forEach(entry => {
if (entry.isDirectory()) {
if (entry.getBaseName() === 'bin' &&
entry.getFile('python').existsSync()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would prefer async here too.

Copy link
Collaborator

@lgeiger lgeiger 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 converting everything to async. 👍

I added one commit that ran prettier on the code and switched from var to let/const.

I think there's still room for simplification here and a small unit test for this functionality would be great but we can iterate on this in future PRs.

@uoo723 Are you interested in helping us maintain this package?

@lgeiger lgeiger merged commit 4b8cfee into atom-community:master Dec 26, 2017
@uoo723
Copy link
Contributor Author

uoo723 commented Dec 27, 2017

I’d be happy to help you with this package.

@lgeiger
Copy link
Collaborator

lgeiger commented Dec 27, 2017

🆒Added you as a collaborator to the repo.

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.

2 participants