-
Notifications
You must be signed in to change notification settings - Fork 29
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
Make auto-workon work with text files containing virtualenv name #45
base: master
Are you sure you want to change the base?
Conversation
@porterjamesj bump ;) |
Whoops, sorry. Been busy; aren't we all -__- I will try to look at this tonight! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! The only other thing is that I'd love to see a test for this before merging.
(when path | ||
(setq venv-current-name path) ;; there's really nothing that feels good to do here ;_; | ||
(venv--activate-dir path)))) | ||
(if (and path (file-regular-p path) (file-readable-p path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment here clarifying what this code does? Doesn't have to be long, I just find if
a bit hard to read in ELisp since there's no explicit else and I don't want to be scratching my head on this code more than necessary somewhere down the road.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment.
Please check if the comment makes it clear what the code does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect!
whose name is in the list `venv-dirlookup-names` it will be | ||
automatically activated. By default, it's value is `'(".venv", "venv")'`, | ||
but you can set if however you like to match your naming conventions: | ||
As long as a virtualenv or a text file with the name of the virtualenv is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, could you check "allow edits from maintainers". There's a couple clarifications I'd like to make to this documentation that are probably easier for me to just do than to explain :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Allow edits from maintainers" is checked. Edit away :)
I'll add a test later this week. Thanks. |
@dakra bump! Are you planning to come back and add a test to this anytime soon? If not I'd like to close just for the sake of tidiness. Github stills seems not happy with me pushing to your fork even thought the "allow commits from maintainers" box is checked, so I can just add the doc fix after we merge if so. |
@porterjamesj Sorry for the late reply. I'm still using it daily. I added a test function but unfortunately it's not working. Maybe you can have look and find out why? Thanks. |
This way you can keep all your virtualenvs in one place and can reuse the same virtualenv for multiple projects. Virtualfish (virtualenvwrapper for fish shell) does this as well, see: http://virtualfish.readthedocs.io/en/latest/plugins.html#auto-activation-auto-activation
This way you can keep all your virtualenvs in one place and can reuse
the same virtualenv for multiple projects.
Virtualfish (virtualenvwrapper for fish shell) does this as well,
see: http://virtualfish.readthedocs.io/en/latest/plugins.html#auto-activation-auto-activation