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

Cleaned up activate.fish. #589

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@jonathanmarvens
Contributor

jonathanmarvens commented Apr 2, 2014

Cleaned up activate.fish to be more idiomatic fish and removed a bit of unnecessary code (such as support for "Aspen magic directories", since AFAIK and AFAICT, this is no longer the case).

- Jonathan

Cleaned up activate.fish.
Cleaned up activate.fish to be more *idiomatic* fish and removed a bit of unnecessary code (such as support for "Aspen magic directories" ... AFAIK and AFAICT, this is no longer the case).

- Jonathan
@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Apr 2, 2014

Member

Is there a reason for de-namespacing the used env variables (e.g _*_VIRTUALENV_* to _*_*)?

For instance, out of context someone might have no idea what or why _OLD_PATH was in their environment for, if they saw at it at some point.

Member

Ivoz commented Apr 2, 2014

Is there a reason for de-namespacing the used env variables (e.g _*_VIRTUALENV_* to _*_*)?

For instance, out of context someone might have no idea what or why _OLD_PATH was in their environment for, if they saw at it at some point.

@Ivoz Ivoz added this to the 1.11.5 milestone Apr 9, 2014

@jonathanmarvens

This comment has been minimized.

Show comment
Hide comment
@jonathanmarvens

jonathanmarvens Apr 9, 2014

Contributor

@Ivoz I agree. I fixed the namespacing issue ... sorry about that. And I apologize for the delay ... I've been super busy. Please let me know if this PR is 👍 now!

Contributor

jonathanmarvens commented Apr 9, 2014

@Ivoz I agree. I fixed the namespacing issue ... sorry about that. And I apologize for the delay ... I've been super busy. Please let me know if this PR is 👍 now!

@jonathanmarvens

This comment has been minimized.

Show comment
Hide comment
@jonathanmarvens

jonathanmarvens Apr 13, 2014

Contributor

@Ivoz Hey, I'm just wondering if everything else is good with this? Thanks.

Contributor

jonathanmarvens commented Apr 13, 2014

@Ivoz Hey, I'm just wondering if everything else is good with this? Thanks.

@jonathanmarvens

This comment has been minimized.

Show comment
Hide comment
@jonathanmarvens

jonathanmarvens Apr 24, 2014

Contributor

@Ivoz Hey, man, I'm just following up. Please let me know if this PR can be merged. Thanks.

Contributor

jonathanmarvens commented Apr 24, 2014

@Ivoz Hey, man, I'm just following up. Please let me know if this PR can be merged. Thanks.

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Apr 25, 2014

Member

@jonathanmarvens I am indeed interested in this; the reason for the lack of merge is that currently virtualenv is left somewhat without active developers with the great depth and breadth of accumulated knowledge needed to confidently merge good changes to such an intricate project.

Currently I want expand virtualenv's tests (such as, enable it to test creating and activating envs with different shells) to mitigate the lack of that knowledge, so, e.g, I can be somewhat confident that changes merged to the fish shell script work even though I've never used fish personally.

Member

Ivoz commented Apr 25, 2014

@jonathanmarvens I am indeed interested in this; the reason for the lack of merge is that currently virtualenv is left somewhat without active developers with the great depth and breadth of accumulated knowledge needed to confidently merge good changes to such an intricate project.

Currently I want expand virtualenv's tests (such as, enable it to test creating and activating envs with different shells) to mitigate the lack of that knowledge, so, e.g, I can be somewhat confident that changes merged to the fish shell script work even though I've never used fish personally.

@jonathanmarvens

This comment has been minimized.

Show comment
Hide comment
@jonathanmarvens

jonathanmarvens Apr 26, 2014

Contributor

@Ivoz Hey man, I totally understand! I can help with that if you'd like! What if I created a Vagrant VM setup specifically for testing the different aspects of virtualenv, so that folks could run that before every merge to check if all is good? Even maybe somehow integrating that with Travis if possible?

Contributor

jonathanmarvens commented Apr 26, 2014

@Ivoz Hey man, I totally understand! I can help with that if you'd like! What if I created a Vagrant VM setup specifically for testing the different aspects of virtualenv, so that folks could run that before every merge to check if all is good? Even maybe somehow integrating that with Travis if possible?

# fish uses a function instead of an env var to generate the prompt.
# copy the current fish_prompt function as the function _old_fish_prompt
if test \( -z $VIRTUALENV_DISABLE_PROMPT \) -o \( -z $VIRTUAL_ENV_DISABLE_PROMPT \)

This comment has been minimized.

@Ivoz

Ivoz Jun 30, 2014

Member

We shouldn't add alternatives for spelling here selectively, especially since its inconsistent with A) other specified public env vars (e.g $VIRTUAL_ENV), and B) the names of those vars for other shells as well.

If you'd like to suggest to rename them, I'd suggest this should be done in a separate PR covering the other activate scripts as well so it would be consistent across the board.

@Ivoz

Ivoz Jun 30, 2014

Member

We shouldn't add alternatives for spelling here selectively, especially since its inconsistent with A) other specified public env vars (e.g $VIRTUAL_ENV), and B) the names of those vars for other shells as well.

If you'd like to suggest to rename them, I'd suggest this should be done in a separate PR covering the other activate scripts as well so it would be consistent across the board.

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Sep 19, 2015

Member

I've applied this in develop, cheers.

Member

Ivoz commented Sep 19, 2015

I've applied this in develop, cheers.

@Ivoz Ivoz closed this Sep 19, 2015

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Sep 20, 2015

Member

@jonathanmarvens turns out removing the double quotes for variables was a really bad thing. They prevent some parameter expansion that can crash the script. Hope you dogfooded your changes before submitting them at the time :/

Member

Ivoz commented Sep 20, 2015

@jonathanmarvens turns out removing the double quotes for variables was a really bad thing. They prevent some parameter expansion that can crash the script. Hope you dogfooded your changes before submitting them at the time :/

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