Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

agnoster theme shows error code instead of an ✘ #1388

Merged
merged 1 commit into from Nov 25, 2012

Conversation

Projects
None yet
Contributor

fuadsaud commented Nov 3, 2012

I think it's more useful to display the error code than just the ✘ when last command failed.

@robbyrussell robbyrussell added a commit that referenced this pull request Nov 25, 2012

@robbyrussell robbyrussell Merge pull request #1388 from fuadsaud/agnoster_theme_show_exit_status
agnoster theme shows error code instead of an "x"
03c2712

@robbyrussell robbyrussell merged commit 03c2712 into robbyrussell:master Nov 25, 2012

Taluu commented Nov 26, 2012

I don't think this is really a relevant change : As for me (and I guess at least for the author of this theme, @agnoster, if he did this choice ?), i don't really care about the return value. And also, it seems your changes are bugged, I have an error command not found : prompt_contextx

So, I had to revert your changes. Sorry. :)

Contributor

fuadsaud commented Nov 26, 2012

hum, I should apologize. I made some typos on the commit that screwed things up :( (I made another pull request fixing it)

The exit code it's not the most useful and necessary thing, but I like it more than having a single character. But that's just me.

Sorry for that, again :\

Taluu commented Nov 26, 2012

No problem. It is like me, as i like in the composer plugin to alter some commands here and there. About that, you make me think i should put this in my .zshrc instead of directly editing the plugin... :)

Contributor

agnoster commented Nov 26, 2012

An earlier version of this theme did indeed have the exit code - I decided in the end against it in the interest of simplicity. I tried to make sure the theme shows no more information than I really need, and 99% of the time I don't need the exit code - I just want to know if a command succeeded or not.

The git status follows a similar principle: many themes show detailed information about whether there are new files, or modifications, or unmerged, etc. etc. Ultimately, conveying too much information can be just as bad as not conveying enough, because it's harder for the user to automatically perceive what they need to know.

So personally, I'd keep it as-is. But I'm biased. ;-)

This change breaks the theme, I think it should be:

prompt_context

Same here, +1

Looks like a typo here as well, should be:

git symbolic-ref HEAD

IMHO this should become an option, rather than a default. I prefer having an "x". So far, I have not been concerned about exit codes; I was only interested in knowing whether a command failed or not.

Not sure why you would even want "x" as an option. A red return code is just as terse and supplies more information. If you care about the exit status at all, then you probably need the return code.

Contributor

fuadsaud replied Nov 28, 2012

That's exactly what I thought @mbulat

I even think the error code gives a better look for the prompt (I don't really like that 'x').

But like I said before, it's not essential, so if people really disagree, this merge should be undone.

+1. The theme is still broken for me.

囧, too young

bimawa commented Dec 14, 2012

i think maybe keep and so and so?
Like this:
[[ $RETVAL -ne 0 ]] && symbols+="%{%F{red}%}✘:$RETVAL"

I'd like to see the return of the ✘ instead of the error code. It's much, much more aesthetically appealing and, given that I rarely if ever need to know the error code, there's no need for it to clutter the command line.

I could write the code change myself & submit the pull request if y'all like; otherwise, I may fork the agnoster theme & start working on my own. Let me know what y'all think.

Contributor

agnoster commented Dec 21, 2012

@jfmercer The sad thing is, I hate the numeric status code. What am I supposed to do, fork it and call it "agnoster's agnoster theme"? Or maybe "agnoster-classic"?

@robbyrussell While I believe very strongly that everyone should have the right to adapt and change my theme however they see fit, it hurts me a little that I put so much work into getting it just so, and now I can't even use my own theme from OMZ. In future, I'd suggest that if people want to modify a theme, they create a new theme forked from the original.

Would you be open to a PR reverting these changes? I totally understand that, since it's a community project you have every right to let people contribute however you see fit, but it would mean a lot to me if I could use my theme straight from OMZ without cringing ;-)

Contributor

fuadsaud commented Dec 21, 2012

@agnoster I agree with you. Themes are personal stuff and they should be customized on forks only.

I still prefer the numeric code, but I'm supporting reverting the changes on upstream.

@agnoster agnoster referenced this pull request Dec 21, 2012

Merged

agnoster "classic" #1510

Contributor

agnoster commented Dec 21, 2012

@fuadsaud Thanks, I can totally see why you'd want the code, it may be that for certain workflows it's more relevant (say, developing CLI tools). I'd like to perhaps re-introduce this as an option, but I don't really want the theme to suffer from optionitis. I tried to make my theme fairly easy to hack so that people could fork it and make their own powerline-style prompts, so I could well imagine building a theme that looks similar, but shows numeric status code, more detailed git information, maybe even some more relevant stuff in the RPROMPT. But I want the agnoster theme to remain focused on brevity and clarity — even if the length and rambling nature of my comments here might suggest I should take some of my own medicine!

I have submitted a pull request, #1510, to revert the numeric code to the X instead.

@w31 w31 pushed a commit to w31/oh-my-zsh that referenced this pull request Apr 30, 2014

@robbyrussell robbyrussell Merge pull request #1388 from fuadsaud/agnoster_theme_show_exit_status
agnoster theme shows error code instead of an "x"
d1fbede
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment