Do not replace already existing completions

[convergedtarkus]

Description

If a user has a completion defined for an alias, the alias completion script should not replace it.

Motivation and Context

I ad a completion for one of my alias and could not figure out why it was not working. Turns out the alias completion logic was replacing my completion. This is helpful for uses who make their own completions.

Technical Explanation

The end goal is for the alias completion script to not replace user provided completions for aliases that otherwise would have completion added by the alias completion script.

This requires a few core changes to achieve and support reload commands such as bash-it reload.

1. Before this change, only some completions added by this script used a wrapper function. Now all completions added by this script use a wrapper.
   • This is needed as the wrapper contains identifying data to tell if this completion was added by the alias completion script. This allows telling the difference between a completion from this script vs one from something else.
2. When running the script, check if the alias being worked on has an existing completion.
   • For those completions defined by the alias completion script, they are removed.
     • While most completions will be replaced by the rest of the script, the old completion does need to be removed first in case the alias has changed and no longer supports a completion function being generated for it. Otherwise an outdated or invalid completion function for the alias will exist on reload.
   • Completions for aliases that are not from the alias completion script will be skipped and the script will not replace it.
     • The assumption here is that if the user, or some other source, has defined a completion for an alias, that completion is more correct than what could be dynamically generated by the script.

How Has This Been Tested?

I verified this with an alias that I had defined a completion for and the alias completion script was adding a completion for. Now it does not replace the one I defined.

This could also be tested by any setup where an alias has a custom completion and the alias script would add a completion for it.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• If my change requires a change to the documentation, I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
• I have added tests to cover my changes, and all the new and existing tests pass.

[seefood]

seems legit, but what happens on a second run of bash-it? won't it ignore updates?

[convergedtarkus]

@seefood Good question, let me look into that.

[convergedtarkus]

@seefood I pushed up a commit that should fix your concern. It does involve having to put all the alias completion functions added by the script into a wrapper so they can be identified on reload and replaced.

[convergedtarkus]

@seefood Just wanted to bump on this.

[seefood]

To tell you the truth, I am not entirely sure what you did there. maybe because I keep getting to it after 11pm or something. I need to read it when my brain is awake.

[convergedtarkus]

@seefood No worries, I understand that.
I pushed up another small fix. I also added a Technical Explanation section in the PR description which provides an overview of the changes. Hopefully that makes the code easier to follow.

[seefood]

seems ok, I trust you tested it... at the very worst we'll revert the PR

[akinomyoga]

Shouldn't this variable be localized?

Also, I'm not sure if one can safely assume that the second last word of complete -p <alias> would be the argument of the -F option. There can be completion settings with -C <cmd> or ones without neither -F nor -C.

[seefood]

You are absolutely right. Adding it as a local var in my next PR.

@convergedtarkus you said you fully tested this, right? Got any input regarding the complete issue raised by Koichi-san?

[convergedtarkus]

I agree on making the variable local.

As for the logic, I think the logic is safe given that the scope is isolated.

This logic checks if the current alias being analyzed has an existing completion. If it does, this command tries to extract the name of the function associated with the completion. If the extracted name is anything but the namespace used for the aliases added by this script, that alias is skipped over because it has an existing completion function that was not added by this script. So the script only cares about aliases with existing completion functions added by this script.

Since this code is only looking for the completion commands added by this script, we can know they are created in a predictable manner. Anything else just ends up being skipped.

[akinomyoga]

Thank you for the explanation. I haven't carefully looked at the code around this line. Checking prefix _${namespace}:: (where namespace is alias_completion) should usually be sufficient. You can mark this conversation as marked (I'm not the PR author or a maintainer, so I don't have access to the resolve button).