Drop LS_COLORS variable #273
Conversation
spaceship.zsh
Outdated
| @@ -91,6 +91,9 @@ SPACESHIP_PROMPT_SUFFIXES_SHOW="${SPACESHIP_PROMPT_SUFFIXES_SHOW:=true}" | |||
| SPACESHIP_PROMPT_DEFAULT_PREFIX="${SPACESHIP_PROMPT_DEFAULT_PREFIX:="via "}" | |||
| SPACESHIP_PROMPT_DEFAULT_SUFFIX="${SPACESHIP_PROMPT_DEFAULT_SUFFIX:=" "}" | |||
|
|
|||
| # LS_COLORS | |||
| SPACESHIP_LSCOLORS_DEFINE="${SPACESHIP_LSCOLORS_DEFINE:=true}" | |||
There was a problem hiding this comment.
SPACESHIP_LSCOLORS_DEFINE 🤔
Option's name should be discussed. Maybe we should use just SPACESHIP_LSCOLORS and assign this variable default value. What do you think?
There was a problem hiding this comment.
Yeah, I'm not super fond of the name too 🙂
Maybe SPACESHIP_USE_LSCOLORS or SPACESHIP_USE_DEFAULT_LSCOLORS?
As for the idea about assigning a default value, I'm not sure that this is the best approach. Note that this theme actually defines two variables (LS_COLORS for Linux and LSCOLORS for Mac), as well as calls zstyle ':completion:*', and if people use a custom provider, all of this is already done and so none of this should be executed again by the prompt theme.
Alternatively we can define LSCOLORS and LS_COLORS if and only if they are undefined yet, that way we don't overwrite anything and yet keep defining the default value for those who don't use any other provider of LS_COLORS. But I'm a bit concerned about this behavior, I'm not as confident that this will not break something for some people. One explicit configuration variable with true as default value seems like a safer bet.
To be honest, this whole LS_COLORS block is a bit surprising for me to see as part of the prompt scheme, usually it's done somewhere else. Providing LS_COLORS is part of either a custom rich provider (like trapd00r/LS_COLORS) or part of framework (like prezto/init.zsh or oh-my-zsh/theme-and-appearance.zsh), while configuring zstyle ':completion:*' for autocompletion coloring is part of autocompletion plugins (like prezto/completion.zsh) or again part of the framework (like oh-my-zsh/theme-and-appearance.zsh).
There was a problem hiding this comment.
@denysdovhan since breaking changes are acceptable in 3.0, I would really recommend to consider removing this whole block exporting LS_COLORS from the spaceship theme, as I said above it doesn't feel like it belongs here.
There was a problem hiding this comment.
@maximbaz I'm working on it 😉
denysdovhan
added
the
improvement
|
What about dropping |
|
I'm completely supporting this! I'll make the change now 🙂 |
denysdovhan
changed the title
Here's why this is needed:
I use a custom provider for LS_COLORS which adds a lot more colors, so I don't want to use the
LS_COLORSfrom this theme. However this theme overrides my definition.You might ask, why don't I simply define my LS_COLORS after this theme is applied? The reason is, it is useful to define proper LS_COLORS in the very beginning of your zshrc, before plugins and themes are loaded, so that for example autocompletion plugin can use this variable and show suggestions in color. I use antigen, which defines both plugins and themes in one place, so I can't really put my own definition of LS_COLORS in between those lines.
I preserved the default value of
true, so nobody will be affected by this change.