-
Notifications
You must be signed in to change notification settings - Fork 743
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
Feature/gitsegment #136
Feature/gitsegment #136
Conversation
p = subprocess.Popen(['git', 'describe', '--tags', '--always'], stdout=subprocess.PIPE, stderr=subprocess.PIPE) | ||
detached_ref = p.communicate()[0].rstrip('\n') | ||
if p.returncode == 0: | ||
branch = '⚓ {}'.format(detached_ref).decode('utf-8') |
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.
There is a better way to do this:
branch = '%c %s' % (u'\u2693', detached_ref)
It's easier to read and it prevents to have a unicode character in the code (also I guess it's a little bit faster since it doesn't call any functions).
@MartinWetterwald : I made a few comments on this PR, do you prefer that I fork your repo, make a PR on it, so you can do a new PR with everything integrated? I hope it will be merged! |
this looks so great! will it be merged? |
@marcioAlmada I have no idea... I hope it will be merged too! |
@MartinWetterwald I forgot to say, your updated regex works fine now :) |
Great work, thanks! What font are you using for your screenshots? The unicode characters look a little funky with the Sauce Code Pro font I'm using. |
Conflicts: segments/git.py
@Anarky Sorry for the late answer! Yes, could you make a PR? |
@Anarky Actually I've just committed the changes you suggested but I've defined a "symbols" variable instead so that all changes will be able to be done at one place. |
@b-ryan I would like to know if you may be interested in merging this pull request. Can you test it out and tell me how do you think about it? I use it for 1,5 year and I really find it useful. You may also test detached HEAD mode (after executing my script, run
|
Hey @MartinWetterwald, sorry for not responding sooner. I have looked through this PR a few times and been unsure what I think. My biggest concern is just over adding too many symbols where it's hard to understand what they mean. In general I am unsure of how to handle dramatic changes like this. I would worry about the majority of people disliking it. There isn't really a great way to know whether that would be the case. There are definitely some things is here I like a lot though, like the unborn branch and showing the tag or sha1 in detached mode. What would you think of not using separate colors for each of the indicators? |
@b-ryan I understand your concern about adding too many symbols and about people disliking it. If the main concern is you fear people may find hard to understand all of the symbols meaning, don't you think that removing the color on the icons would make it worse? Color adds meaning. I've tried to use intuitive icons and colors. The colors reflects more or less the default colors shown by git status (the green for example means stagged for commit). I agree it's hard to know how many people would like it. This is because only a minority of powerline-shell users is involved here in the discussions. If you like it yourself we could merge the PR and if later there is too many people disliking it, we could
How do you think? |
It would be nice if this could be refactored so that each unique "segment" in your screenshot is actually a separate segment in |
@phatblat it is a nice idea, but at the moment one of the largest problems with that is that each segment would need to shell out to @MartinWetterwald what you said makes sense. I will give this code a run and come back with any issues. Assuming there are none, I'd like to merge this maybe tonight to keep if from being delayed further. |
if line.find('Untracked files') >= 0: | ||
has_untracked_files = True | ||
return has_pending_commits, has_untracked_files, origin_position | ||
def get_git_status(pdata): |
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.
pdata
is not great name. Maybe status_data
instead?
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.
Or maybe just move the git status
call back into this function. Doesn't look like pdata
is being used elsewhere.
From what I can see and from the testing I have done, everything looks good. There are a few refactorings that I think this change could benefit from, but I won't hold things up. I was just playing around in the code and can push up my own branch with those refactors. I think the only thing that should be added before merging is something to the README to describe the symbols. I will look into doing that right now. |
Comments welcome on #207 |
Thanks @MartinWetterwald |
This is so nice. I ❤️ the icons! Nice work everyone! |
I'm really happy you decided to merge it. :) |
Hello,
This branch enhances the git segment with detailed "git status" overview in the prompt.
For the prompt to still be fast, I tried my best to minimize the number of shell commands launched.
In your version, 2 git processes are launched, in my version, I succeed to keep the same number: I only launch 2 git subprocesses, but there is much more info displayed.
I still don't like the default color I chose for "modified but not staged for commit". Do you have a better idea?
This is the longest prompt possible:
