-
Notifications
You must be signed in to change notification settings - Fork 401
fix: completion generation inside $() #1403
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: main
Are you sure you want to change the base?
Conversation
Command substitution completion was incorrectly setting the `prev` variable due to the `_comp_initialize` function reassigning it from the original completion context after processing the substitution. The fix ensures that `words` and `cword` variables are also updated to match the command substitution context, preventing the later reassignment from overriding the correct `prev` value. Unit test added to validate command substitution correctly sets `COMP_LINE`, `COMP_CWORD`, `cur`, and `prev` variables for both single commands and commands with arguments.
2b82879 to
550e0cd
Compare
yedayak
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.
In main echo $(ip <TAB> completes all files (the default completion, it should complete sub-commands), but with this PR it doesn't complete anything. Maybe you can use _comp_command_offset for this?
| # Replace the completion variables with ones that reflect the inner context. | ||
| COMP_LINE=$inner_line | ||
| read -ra COMP_WORDS <<<"$inner_line" | ||
| COMP_CWORD=$((${#COMP_WORDS[@]} - 1)) |
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.
This can become negative if there isn't anything inside the $():
echo $(<TAB>bash: COMP_WORDS: bad array subscript
|
@yedayak Thanks for the quick review. I'll investigate. |
scop
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.
Just a note to set "changes requested" per earlier comments so this shows up as such in PR list.
Closes #630
python -m pytest test/t/unithas two failing tests and two skipped tests intest_unit_compgen_filedir.py. This is the same as onmain, so I do not believe they are related to this patch.python -m mypy .andpython -m ruff checpassI am unsure what to make of the
runLintoutput, but it has exit status0, so I presume it is OK