search

ohmybash / oh-my-bash

A delightful community-driven framework for managing your bash configuration, and an auto-update tool so that makes it easy to keep up with the latest updates from the community.
https://ohmybash.github.io
MIT License
5.53k stars 624 forks source link

lib/bourne-shell.sh: Don't force-load bash-completion #572

Open ifireball opened 1 month ago

ifireball commented 1 month ago

Its already done better by the "system" completion module, and doing it there too makes disabling it impossible and debgging issues harder.

ifireball commented 1 month ago

@akinomyoga is there a way to perhaps print a warning message during upgrade?

Wasn't "system" in the default ~/.bashrc template already for a while now? I was sure I wasn't the one that put it there in my local file...

If we absolutely want to keep the current behaviour, we can:

  1. Add an small hack in oh-my-bash.sh to force add system to the completions array.
  2. Have a variable to switch that hack off.
  3. Set the variable by default in the ~/.bashrc template.

That way newer users and those who explicitly add the variable it can get the new behaviour while users with older ~/.bashrc keep the existing behaviour.

We can call the variable something like _compat_force_load_system_completion to indicate to people this may be a temporary thing that will eventually go away.

ifireball commented 1 month ago

Please let me know if you want me to add what I've described. Since you've modified the PR I don't want to make further changes without your approval

akinomyoga commented 1 month ago

@akinomyoga is there a way to perhaps print a warning message during upgrade?

It seems no. I've checked the code to update OMB. The upgrading is performed by a script in the older version of OMB (before upgrading), and currently, there doesn't seem to be a way to change the behavior for the new version of OMB after upgrading.

Wasn't "system" in the default ~/.bashrc template already for a while now?

No. It's never been there. From the beginning when the completions array is added to the template, its content has been git, composer, and ssh, and has never been changed.

I was sure I wasn't the one that put it there in my local file...

I'm not sure...

If we absolutely want to keep the current behaviour, we can:

  1. Add an small hack in oh-my-bash.sh to force add system to the completions array.
  2. Have a variable to switch that hack off.
  3. Set the variable by default in the ~/.bashrc template.

That's technically possible, but if we would have such a variable, I'd rather do that the other way around. That is,

  1. We keep the code in lib/bourne-shell.sh
  2. Add a variable to turn off loading bash-completion in lib/bourne-shell.sh

Please let me know if you want me to add what I've described. Since you've modified the PR I don't want to make further changes without your approval

I want to think about it more.

ifireball commented 1 month ago

That's technically possible, but if we would have such a variable, I'd rather do that the other way around. That is,

  1. We keep the code in lib/bourne-shell.sh
  1. Add a variable to turn off loading bash-completion in lib/bourne-shell.sh

That will mean we keep having duplicate code. Perhaps we can do one of:

  1. Source completions/system.completion.sh from lib/bourne-shell.sh.
  2. Put the code in a function defined in lib/bourne-shell.sh and then call it from lib/bourne-shell.sh (Conditioned by a variable) and from completions/system.completion.sh

I want to think about it more.

Go ahead. Perhaps consider adding a mechanism to inform users about changes they may want to make to their configuration after an upgrade. Having that would have been handy now. If you want, I can try to write down an issue to describe what I think it aught to look like and we can discuss the details there.