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

Added conda env segment #24

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Added conda env segment #24

wants to merge 7 commits into from

Conversation

Gigitsu
Copy link

@Gigitsu Gigitsu commented Dec 27, 2016

No description provided.

@drorata
Copy link

drorata commented Aug 2, 2017

I guess you also need to add

changeps1: False

to .condarc.

env=$VIRTUAL_ENV
fi

if [[ -n $CONDA_DEFAULT_ENV ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

why not check this first and then use elif to explicitly fall back? This will make sure that this is the first choice if changes are made in the future.

Copy link

Choose a reason for hiding this comment

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

@MichaelAquilina I assume you're right. As this is not my PR I cannot say for sure. But it makes sense.

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean something like this?

if [[ -n $CONDA_DEFAULT_ENV ]]; then
  env=$CONDA_DEFAULT_ENV
elif [[ -n $VIRTUAL_ENV ]]; then
  env=$VIRTUAL_ENV
fi

Copy link
Contributor

Choose a reason for hiding this comment

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

yes :) @Gigitsu

Copy link
Contributor

@MichaelAquilina MichaelAquilina Aug 4, 2017

Choose a reason for hiding this comment

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

also, its good practice to wrap your variables in quotes, otherwise spaces might give you wrong results:

if [[ -n "$CONDA_DEFAULT_ENV" ]]; then
  env="$CONDA_DEFAULT_ENV"
elif [[ -n "$VIRTUAL_ENV" ]]; then
  env="$VIRTUAL_ENV"
fi

Copy link
Author

Choose a reason for hiding this comment

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

Ok, it make sense to me too :)

Copy link
Author

Choose a reason for hiding this comment

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

@MichaelAquilina thank you for the advices, I've made the changes and updated the pull request

@Gigitsu
Copy link
Author

Gigitsu commented Aug 4, 2017

@drorata thank you for your interest.

I don't thinks it is necessary to set changeps1 to false, I have that property to true and it works fine :)

@drorata
Copy link

drorata commented Aug 4, 2017

@Gigitsu I had to disable changeps1 in my .condarc...

@Gigitsu
Copy link
Author

Gigitsu commented Aug 4, 2017

@drorata what behaviour do you have with changeps1 enabled?

@drorata
Copy link

drorata commented Aug 4, 2017

screen shot 2017-08-04 at 14 57 33

The first (non colored) part comes from Conda's patching of $PS1. At least this is what I understand.

@drorata
Copy link

drorata commented Aug 6, 2017

I can confirm that also on another system I had to disable changeps1.

drorata added a commit to drorata/oh-my-zsh that referenced this pull request Aug 6, 2017
if [[ -n $VIRTUAL_ENV ]]; then
local env='';

if [[ -n "$CONDA_DEFAULT_ENV" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

looks good. But probably worth adding a comment explaining why this code exists :)

fi
}

prompt_kubecontext() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

kubernetes is unrelated to conda. Want to break this out to a separate PR?

@apjanke
Copy link
Collaborator

apjanke commented Jan 3, 2020

If y'all are interested, I added support for this over on my AgnosterJ fork.

@HaHeho
Copy link

HaHeho commented Aug 20, 2020

I can confirm that also on another system I had to disable changeps1.

Same here. I mean this modification takes no influence on the on the .condarc configuration. Although it is clearly desired to have latter disabled if agnoster should show the env. But I have no idea if that is reasonable to implement.

BTW, I would be very happy to get this merged and integrated (if the requested changes could be made).

@Gigitsu
Copy link
Author

Gigitsu commented Aug 21, 2020

I can confirm that also on another system I had to disable changeps1.

Same here. I mean this modification takes no influence on the on the .condarc configuration. Although it is clearly desired to have latter disabled if agnoster should show the env. But I have no idea if that is reasonable to implement.

BTW, I would be very happy to get this merged and integrated (if the requested changes could be made).

Ok, I've created another pull request ( #148 ) for k8s context, do you prefer a new pr for this to or can I just modify this?

@michcio1234
Copy link

Anything happening here? :)

@ehartford
Copy link

Can this go in please?

@damonhayhurst
Copy link

Ditto

@Gigitsu
Copy link
Author

Gigitsu commented Aug 27, 2023

Hi @MichaelAquilina, can I do something to get this PR approved? If not, we can go ahead and close it.

@tuhinsharma121
Copy link

@Gigitsu thanks for the work. It works perfectly in my system. I dont understand why its not merged yet. I had to use your fork. Here I have attached a screenshot.
Screenshot 2024-05-04 at 8 09 40 AM

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

Successfully merging this pull request may close these issues.

9 participants