oh-my-fish / theme-agnoster

MIT License
96 stars 64 forks source link

Mercurial coloring fix #31

Closed mtrp12 closed 6 years ago

mtrp12 commented 6 years ago

Support for mercurial was not working out of the box on Ubuntu 18.04. Thus this pull request. Some more justification in comment section.

sn0cr commented 6 years ago

Hi @mtrp12,

could you include the now changed master and then I'll can merge it.

mtrp12 commented 6 years ago

Originally, the feature of the prompt, if I understand correctly, was to show red background for add/remove/modify/missing(removed by os command but still tracked), yellow if there are only untracked files in the directory and green for others(nothing to do). In my pull request, i combined untracked option with first condition, that is add/remove etc. I did it so that it behaves like the git prompt in the same file. Do you accept this change? The update will be different based on your decision.

Also, after looking at the new code in master, i feel like the author misinterpreted the meaning of "!" symbol. In mercurial it means missing file only. But the symbol is not returned here by a mercurial command but an extension namely hg-prompt. According to the wiki of the extension, mentioned in a previous comment, "!" also signifies add/modify. So, while at the end it works out for the user, it may be confusing for other reviewers. I would like to propose changing _color_hg_removedbg to _color_hg_changedbg.

I will be awaiting your comment.

PS: For now I included the necessary changes, assuming your decision to be 'yes' and 'ok' respectively. I am new to git/github, so, let me know if I have done it incorrectly.

sn0cr commented 6 years ago

Your changes are looking good, however we have some merge conflicts, could you resolve them and push again? (i.e., git merge master and here you are free to keep the commit message 😉)

mtrp12 commented 6 years ago

I already merged master before. Why did it cause conflict? As you can see the latest merge commit has 0 changes.

tkindy commented 6 years ago

@sn0cr any word here? Looks like those merge conflicts were fixed.