twolfson / sexy-bash-prompt

Bash prompt with colors, git statuses, and git branches.
MIT License
1.14k stars 155 forks source link

Prefixed .bash_prompt's functions with prompt_ #40

Closed luizberti closed 10 years ago

luizberti commented 10 years ago

Refactored .bashprompt functions to contain the prefix `prompt`.

This is aimed to increase cosistency throughout the project, along with all of the variables that already contain the prefix. This will also help reduce conflicts with shell autocompletion, since all functions are being exported to the environment and can be invoked from an interactive shell.

luizberti commented 10 years ago

I apologize but my commit broke the test suite, will need to refactor that as well

rpdelaney commented 10 years ago

Glad to know I'm not the only one who sent a PR before running the tests :smirk:

twolfson commented 10 years ago

Thoughts on making the prompt_ prefix more descriptive of the repo itself (e.g. sexy_bash_prompt or sbp_)? I know in Sublime Text this notation is preferred since all commands occupy a global namespace (e.g. SideBarEnhancements has side_bar_ for all their commands).

https://github.com/titoBouzout/SideBarEnhancements/blob/37da75704bc1968d65957158318a4b283be89291/Commands.sublime-commands

luizberti commented 10 years ago

@twolfson I thought prompt_ was a good idea given that most (all?) variables on the project are already being defined with that prefix, so it would constitute a standard prefix throughout the whole project. Making the prefix more descriptive would make sense were we to include it to the rest of the project's variables as well (safe for some uppercase ones used for config, colors, etc... makes more sense for those to keep the prefix PROMPT_ for easier identification/maintence among larger .bashrc/.bash_profile files)

@rpdelaney Yeah, sorry about that... :disappointed:

I am currently having no luck in pinpointing why PS1 is failing all tests, would you care to help me out? It's failing all test cases for expected prompt, and I am not nearly as familiar with the project as you...

# ERRORS OCCURRED. STDERR OUTPUT:
`PS1` is not as expected (256) `PS1` is not as expected (8) `PS1` is not as expected (ANSI) `PS1` is not as expected (overridden)
make: *** [test] Error 1
twolfson commented 10 years ago

@luizfb I am okay with creating a breaking major change for better namespacing but I guess prompt_ is good enough. Fwiw, it is usually best to address namespacing issues sooner rather than later since more users have to adjust when they crop up.

twolfson commented 10 years ago

@luizfb I took a look at your failing tests. There are comments above them on how to debug that finnicky section:

https://github.com/twolfson/sexy-bash-prompt/blob/0.22.0/test/prompt_test.sh#L243-L247

It looks like you missed a function rename in your test suite:

https://www.diffchecker.com/j63vkhlx

When we correct it, the tests are green:

https://github.com/twolfson/sexy-bash-prompt/commit/797ef334656b77b8c1a95d2b2f1afd6c69cd77d8

https://travis-ci.org/twolfson/sexy-bash-prompt/builds/34360819

rpdelaney commented 10 years ago

As long as we're doing the renaming, we might as well use the most unique naming scheme possible with the least chance of collisions, no?

twolfson commented 10 years ago

Alright, let's pull the trigger on the prefix rename for existing variables then. What do we want the prefix to be?

SEXY_BASH_PROMPT_BACKGROUND_COLOR
SEXY_PROMPT_BACKGROUND_COLOR
SEXY_BASH_BACKGROUND_COLOR
SBASH_PROMPT_BACKGROUND_COLOR
SBP_BACKGROUND_COLOR
# or something else
twolfson commented 10 years ago

My vote is for the full sexy_bash_prompt_ prefix. It might make quick work of 80 character limits but it is explicit and distinct.

luizberti commented 10 years ago

I guess any prefix starting with sexy would be perfect, there are probably no other programs as sexy as this that have the prefix sexy to collide names with.

But still I think it would be smart to leave the following prefixed the way they are for a couple of reasons. It keeps .bash_profile configurations cleaner, since the name is shorter and more objective. The other reason would be that it would break user's current configuration (albeit being easy to fix).

PROMPT_USER_COLOR
PROMPT_PREPOSITION_COLOR
PROMPT_DEVICE_COLOR
PROMPT_DIR_COLOR
PROMPT_GIT_STATUS_COLOR
PROMPT_GIT_PROGRESS_COLOR
PROMPT_SYMBOL_COLOR
rpdelaney commented 10 years ago

Also, it would be worthwhile to consider that any collisions with PROMPT_USER_COLOR and the like that did occur may be intentional -- unlikely as it may seem, someone down the road might like the idea of re-using these configuration options in another custom styled prompt. That would make migration relatively easy.

As for the fuctions, I agree with @twolfson. Long, readable function and variable names are the best. We don't have to work within 8 bit registers anymore. :+1:

twolfson commented 10 years ago

I have revived this PR in #44 since it seemed to stagnate since last time =/

twolfson commented 10 years ago

This has been merged and released in 0.24.0