Skip to content
This repository was archived by the owner on Apr 24, 2020. It is now read-only.

Conversation

@nipunsadvilkar
Copy link

@nipunsadvilkar nipunsadvilkar commented Oct 12, 2018

Showing python version would be helpful!

screen shot 2018-10-12 at 5 42 34 pm

@dritter
Copy link
Member

dritter commented Oct 12, 2018

Hi @nipunsadvilkar !

Thanks for your Pull Request. New features should be developed on next branch.

And regarding the PR itself, I have a tendency towards creating separate version segments that could be joined. In this case we have several segments concerning python frameworks, which would probably want to show the python version. And probably some people would want to hide/show this version, so we would end up adding the python version and switches in all of these segments, instead of making one python_version segment and joining that one..
WDYT @bhilburn ?

@nipunsadvilkar nipunsadvilkar changed the base branch from master to next October 12, 2018 18:53
@nipunsadvilkar
Copy link
Author

nipunsadvilkar commented Oct 12, 2018

@dritter: Sorry didn't know about which base branch to consider.

have changed the base branch from master -> next. though not sure why there is merge conflict!

Regarding your point about some people like to hide/show version is fair point. I just thought adding such feature just in case someone is interested.

Thanks for an update!

@dritter
Copy link
Member

dritter commented Oct 16, 2018

The conflict is there, because we split all segments in separate files in next and changed variable prefixes to P9K_ .. So you need to move your changes to segments/anaconda.p9k, and change the variable names accordingly.

Regarding your point about some people like to hide/show version is fair point. I just thought adding such feature just in case someone is interested.

As said, we have a mechanism of joining segments together. This would make the feature more flexible, because every user can output the python version before or after any other python framework segment. That is why I am pro separate segment.

@hududed hududed mentioned this pull request Oct 27, 2018
@bhilburn
Copy link
Member

Sorry for the slow response, here. Okay, I have a few thoughts:

  1. I'm generally on the same page as @dritter - separable functionality should be in different segments to make things easily configurable.
  2. The exception to that case, which we have elsewhere in P9k, is if the integration is basically free. So, for example, if you're in an Anaconda environment it sets a bunch of env variables (e.g., $CONDA_PATH), which are being read at the top of this function. If there is an env variable set in an Anaconda environment that gives us the Python version, then sure, we can grab that too and add it to the segment - that avoids a call out to an external program (python), and integrates easily.

That said, this would also need to be configurable - so, the segment would need a flag added to it for whether or not to also show the Python version in the current environment.

@nipunsadvilkar - I think this is a really good idea, and honestly I would use this feature. Are you up for implementing the above and porting your code to the next branch?

@bhilburn bhilburn added enhancement next good for beginner Hacktoberfest Recommended Issue for Hacktoberfest! labels Oct 30, 2018
@nipunsadvilkar
Copy link
Author

@bhilburn : Yeah sure! Happy to contribute.

It would be nice if you could list down required tasklist which needs to be completed to merge this PR.

@dritter
Copy link
Member

dritter commented Nov 4, 2018

The tasks would be:

  • Create a new segment for the python version based on next branch

About the merge conflict: It would be the easiest to create a new PR..

@bhilburn
Copy link
Member

bhilburn commented Nov 5, 2018

I'm going to close this PR in favor of #1019

@bhilburn bhilburn closed this Nov 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants