-
-
Notifications
You must be signed in to change notification settings - Fork 749
feat(lib/omb-prompt-base): Add support for distrobox #683
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
base: master
Are you sure you want to change the base?
Conversation
|
I'm not familiar with this field, so I don't have a way to tell whether the suggested change does the right thing. My naive question is whether the environment variable |
|
ChatGPT says CONTAINER_ID is not a signal for Distrobox: ChatGPT-CONTAINER_ID.pdf, but I'm not sure. |
|
Another thing is whether the Distrobox information in a prompt has a significant demand by a large number of users. For example, Starship (which provides a wide range of options to show various information) doesn't seem to offer a module for Distrobox. Instead, Starship seems to include module |
akinomyoga
left a comment
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.
If we decide to include the suggested Distrobox information, small adjustments are needed.
- Could you check the use of
OMB_PROMPT_SHOW_SPACK_ENVand prepare a similar variable for the Distrobox information? We don't want to change the default behavior of the font theme as long as the old default doesn't cause problems. In the present case, we haven't shown the Distrobox information by default, and the existing users of thefonttheme might not want to show the Distrobox information in the prompt. If thefonttheme starts to show it by default and there is not way to turn off, it could be a trouble for existing users. To avoid such a conflict, we want to introduce a variable to enable/disable the prompt information.
| distrobox_container= | ||
| [[ ${CONTAINER_ID-} ]] || return 1 | ||
| distrobox_container=${CONTAINER_ID} | ||
| _omb_prompt_format distrobox_container "$distrobox_container" OMB_PROMPT_DISTROBOX:DISTROBOX_THEME_PROMPT |
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.
| _omb_prompt_format distrobox_container "$distrobox_container" OMB_PROMPT_DISTROBOX:DISTROBOX_THEME_PROMPT | |
| _omb_prompt_format distrobox_container "$distrobox_container" OMB_PROMPT_DISTROBOX |
The name after : is used to process a deprecated form of the format specifier <name>_PREFIX and <name>_SUFFIX. However, this prompt information is a new one and there hasn't been deprecated variables DISTROBOX_THEME_PROMPT_PREFIX and DISTROBOX_THEME_PROMPT_SUFFIX. We don't want to add new deprecated variables that have never been used.
I'm no expert either and am new to both
This might be a problem... From GitHub stars count there is a considerable user base to distrobox but not that much. Would you recommend moving the implementation into a plugin instead of the base lib? |
OK. It seems to be the convention for Docker managers to create the variable One option is to rename this to a more general name, |
|
This would be really helpful. The same it is already for pyenvs with |
Distrobox is "a container wrapping layer that allows the user to install containerised versions of Linux that are different to the host while providing tight integration with the host allowing the use of binaries designed for one distribution to run on another (Arch wiki)".
This pull request implements the basic function to get the distrobox container name and modifies the default
fonttheme to add a distrobox information field.