bash-completion icon indicating copy to clipboard operation
bash-completion copied to clipboard

Use _comp_split and _comp_compgen in completions/*

Open scop opened this issue 1 year ago • 6 comments

Now we have a utility function _comp_split to safely split strings (including the output of the compgen builtin and other commands) without being affected by pathname expansions, unexpected word splitting, etc. [...] The changes to completions/* are left for later PRs.

_Originally posted by @akinomyoga in https://github.com/scop/bash-completion/pull/919

scop avatar May 01 '23 04:05 scop

Naming of functions that generate completions in COMPREPLY

Ref: Originally raised as a part of https://github.com/scop/bash-completion/discussions/539#discussioncomment-3555283

I would like to discuss the naming of a particular class of functions (including _filedir, _parse_help, and _parse_usage).

For this rewriting, we will switch from COMPREPLY=($(compgen ...)), etc. to use _comp_compgen .... One of the frequent cases among them is COMPREPLY=($(compgen -W '$(_parse_help ...)' -- "$cur")). I would like to rewrite them to use new versions of _parse_help or _parse_usage that directly assign results to COMPREPLY (or a specified array).

Then there is a related discussion that I wanted to open. Currently, there are utility functions of the name _<CATEGORY> that generate and store completions of the category in COMPREPLY, such as _filedir, _ip_addresses, and _pids. The functions _parse_help and _parse_usage also behave in the same way after the suggested change to store results in arrays. If we just follow the new naming convention, the simple prefixing of _comp works for all of them (such as _comp_filedir).

  • _comp_filedir, _comp_ip_addresses, _comp_pids, ...

But I think there is also a possibility to introduce a namespace or a naming rule for this class of functions, such as

  • _comp_generate_filedir, _comp_generate_ip_addresses, _comp_generate_pids, ...
  • _comp_gen_filedir, _comp_gen_ip_addresses, _comp_gen_pids, ... (shorter name)
  • _comp_yield_filedir, _comp_yield_ip_addresses, _comp_yield_pids, ... (typical word for generator/coroutine)
  • _comp_reply_filedir, _comp_reply_ip_addresses, _comop_reply_pids, ... (inspired by the variable name COMPREPLY)
  • _comp_action_filedir, _comp_action_ip_addresses, _comp_action_pids, ... (inspired by compgen -A <action>)
  • _comp_compgen_filedir, _comp_compgen_ip_addresses, _comp_compgen_pids, ... Note: we already use this namespace as the function _comp_compgen, but the default behavior of _comp_compgen is different from existing _filedir, etc.: E.g., _comp_compgen requires an array name and doesn't filter completions by -- "$cur" by default. Maybe we could also adjust the default behavior of _comp_compgen this time.
  • or other ways

What do you think?

edit: Now I'm leaning to _comp_compgen_* with the _comp_compgen interface (#951). #952 is the basic implementation, and _parse_{help,usage} are also implemented (#954) based on #952.

Interface change of _comp_compgen (#951)

edit: I'm currently thinking of changing the interface of _comp_compgen so that it behaves similarly to _filedir, _ip_addresses, _pids, etc., i.e., it stores the results to COMPREPLY by default and filters the completions by -- "${cur-}". This is because the most of the calls of _comp_compgen currently in master and in the planned PRs have the pattern _comp_compgen COMPREPLY ... -- "$cur".

# currently
_comp_compgen [-al|-F sep] ARRAY COMPGEN_ARGS... -- CUR

# suggestion (By default, ARRAY is _comp_compgen, and CUR is "${cur-}")
_comp_compgen [-al|-F sep|-c CUR|-v ARRAY] -- COMPGEN_ARGS...

akinomyoga avatar May 01 '23 06:05 akinomyoga

Also, PR #560 by @algorythmic is related to _parse_help, which would need to be first merged before applying the interface change of _parse_help. I have rebased #560. Could you first check #560?

akinomyoga avatar May 01 '23 11:05 akinomyoga

I added another PR #950, which I'd like to address before _parse_{help,usage}.


I'd like to separate related PRs for easier review. There are some dependencies between the planned changes.

  • _parse_help-3 #971
  • _parse_help-2 #969
    • _parse_help-1 #954
      • #560
      • #950
      • #951
      • naming of functions that generate completions in COMPREPLY #948 (comment)
      • #952
  • #955
  • #970
  • #972
  • #973
  • #985
  • #989
    • Resolve #958
  • #994

akinomyoga avatar May 01 '23 21:05 akinomyoga

@akinomyoga what are your thoughts on the status of this? For example a quick git grep -F "COMPREPLY=(" reveals a few dozen hits I think we could look into before 2.12.

scop avatar Dec 31 '23 10:12 scop

As I have replied once in https://github.com/scop/bash-completion/discussions/530#discussioncomment-6642445, I wouldn't request it to be part of 2.12. However, if you like it, I'll try to look at it to see whether it is easy to apply. If it would cause again a bunch of discussions, we can probably postpone applying it not to delay the new release further.

akinomyoga avatar Dec 31 '23 11:12 akinomyoga

I have submitted PRs that replace unquoted array assignments.

  • #1083
  • #1084
    • #1085
      • #1086
      • #1087

akinomyoga avatar Jan 01 '24 22:01 akinomyoga

Removing this from the 2.12 TODO list, this is not an API issue nor essential for the release, and could take a bit of time.

scop avatar Feb 01 '24 20:02 scop

Thank you for reviewing all those issues!

I actually thought the PRs mentioned above were the last ones for replacing arr=($word) and compgen with _comp_split and _comp_compgen. The generators of the form _comp_compgen NAME were not initially intended for this issue because the generators were introduced in #952 after this issue was raised. I feel we can now close this issue. But maybe the confirmation itself will take time, so we can remove this from the ToDo list anyway.

akinomyoga avatar Feb 01 '24 21:02 akinomyoga

Thanks for the feedback, I'll close this one now per that. I'd like to be able to reduce the number of open issues and PR's to ones for which there is clear intent to work on addressing something in the foreseeable future.

scop avatar Feb 02 '24 07:02 scop