Skip to content

fix: quote $split of $split && return for custom IFS #918

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

Merged
merged 3 commits into from
Apr 25, 2023

Conversation

akinomyoga
Copy link
Collaborator

@akinomyoga akinomyoga commented Apr 8, 2023

These are all the following simple changes but just applied to many files.

-    $split && return
+    "$split" && return

This was originally a part of #914, but I have separated it for easier review of non-trivial changes in #914.

@akinomyoga
Copy link
Collaborator Author

Do we also switch to [[ $split ]] for split? This involves an interface change to _comp_initialize, but we have renamed it from _init_completion so we may write an adapter in 000_bash_completion_compat.sh.

@akinomyoga
Copy link
Collaborator Author

From #931 (comment) by @scop
Now that we're doing the boolean checks differently throughout for other cases, the _comp_initialize $split case looks inconsistent. I wonder if it would make sense and be worth the trouble to introduce another variable, e.g. $was_split or something with the empty/set value space we use for the other boolean checks? Would be a big diff though. Perhaps there would be an opportunity to do something about this as we've renamed _init_completion to _comp_initialize. What do you think?

@akinomyoga
Copy link
Collaborator Author

@scop
Copy link
Owner

scop commented Apr 23, 2023

Ooh ❤️ thanks heaps! I think I'd prefer option C -- it reads slightly better to me, and could cause less confusion for 3rd party completion writers, in case they take some of our existing completions as a template but opt to use _init_completion for backwards compatibility. Which option would you prefer?

Let's just add a @deprecated note and elaboration to the new _init_completion wrapper.

@akinomyoga akinomyoga force-pushed the quote-split branch 2 times, most recently from 7a1907d to 44c4327 Compare April 23, 2023 09:26
@akinomyoga akinomyoga marked this pull request as draft April 23, 2023 09:26
@akinomyoga akinomyoga marked this pull request as ready for review April 24, 2023 21:32
@akinomyoga
Copy link
Collaborator Author

akinomyoga commented Apr 24, 2023

I pushed the version for option C (was_split).

Let's just add a @deprecated note and elaboration to the new _init_completion wrapper.

I added the original description and @deprecated with the way to migrate.

Copy link
Owner

@scop scop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@scop scop merged commit ea39680 into scop:master Apr 25, 2023
@akinomyoga akinomyoga deleted the quote-split branch April 25, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants