Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Complete command line arguments #294

Merged
merged 16 commits into from
Aug 11, 2021
Merged

Complete command line arguments #294

merged 16 commits into from
Aug 11, 2021

Conversation

otreblan
Copy link
Contributor

@otreblan otreblan commented Jun 9, 2021

Closes #117.

bash.mp4

In vim, - must be added to the keyword characters for this to work.

autocmd FileType sh set iskeyword+=45

@codecov
Copy link

codecov bot commented Jun 9, 2021

Codecov Report

Merging #294 (bf5e329) into master (b8c0c64) will increase coverage by 0.25%.
The diff coverage is 80.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #294      +/-   ##
==========================================
+ Coverage   74.04%   74.30%   +0.25%     
==========================================
  Files          18       18              
  Lines         551      576      +25     
  Branches       88       94       +6     
==========================================
+ Hits          408      428      +20     
- Misses        124      126       +2     
- Partials       19       22       +3     
Impacted Files Coverage Δ
server/src/server.ts 63.58% <80.00%> (+1.45%) ⬆️
server/src/analyser.ts 83.23% <81.81%> (-0.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a16b288...bf5e329. Read the comment docs.

@skovhus
Copy link
Collaborator

skovhus commented Jun 9, 2021

@otreblan thanks for adding this! 👏 Do you have time to add a few unit tests? That would be great.

@otreblan
Copy link
Contributor Author

otreblan commented Jun 9, 2021

@otreblan thanks for adding this! clap Do you have time to add a few unit tests? That would be great.

Done

Copy link
Collaborator

@skovhus skovhus left a comment

Choose a reason for hiding this comment

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

Thank you so much for contributing here. This would be a great addition to the language server.

The get-options.sh shell script doesn't seem that portable. I'm wondering if we can come up with a more portable/cross platform solution. Note the we already have the man and help page that we could parse.

Another thing: updating the triggerCharacters configuration option (line 107 in src/server.ts) to include - makes it work without additional configuration.

@otreblan
Copy link
Contributor Author

Another thing: updating the triggerCharacters configuration option (line 107 in src/server.ts) to include - makes it work without additional configuration.

When - is in triggerCharacters:

trigger.mp4

@@ -1,6 +1,8 @@
#!/usr/bin/env bash

source /usr/share/bash-completion/bash_completion
DATADIR="$(pkg-config --variable=datadir bash-completion)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does the script handle if the user doesn't have the completion installed?

~ pkg-config --variable=datadir bash-completion

package bash-completion was not found in the pkg-config search path.
Perhaps you should add the directory containing `bash-completion.pc'
to the PKG_CONFIG_PATH environment variable
No package 'bash-completion' found

~ echo $?
1

Copy link
Collaborator

@skovhus skovhus Jul 4, 2021

Choose a reason for hiding this comment

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

At least on OS X the suggested solution doesn't work. Does the solution works on most Linux distributions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should work if the bash-completion package is installed and it provides a bash-completion.pc file.

Copy link
Collaborator

@skovhus skovhus left a comment

Choose a reason for hiding this comment

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

Thanks for adding this!

@skovhus skovhus merged commit 2b4d8a6 into bash-lsp:master Aug 11, 2021
Comment on lines +24 to +29
# Disabled by default because _longopt executes the program
# to get its options.
if (( ${BASH_LSP_COMPLETE_LONGOPTS} == 1 ))
then
_longopt "${COMP_WORDS[0]}"
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw _longopts is disabled until BASH_LSP_COMPLETE_LONGOPTS=1, because it can execute an arbirary program.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, so we require people to set BASH_LSP_COMPLETE_LONGOPTS=1 in their bash profile, or?

@gegoune
Copy link

gegoune commented Aug 11, 2021

Should it work when editing sh file (with #!/usr/bin/env bash) in neovim started from zsh? I have installed bash-completion package with homebrew, but it does not provide completions on find -. bashls updated to 2.0.0 and iskeyword contains - (45).

@otreblan
Copy link
Contributor Author

Should it work when editing sh file (with #!/usr/bin/env bash) in neovim started from zsh? I have installed bash-completion package with homebrew, but it does not provide completions on find -. bashls updated to 2.0.0 and iskeyword contains - (45).

Do you have bash-completion@1 or bash-completion@2?

@gegoune
Copy link

gegoune commented Aug 11, 2021

Version 1. Works with @2, thank you! Although I am only getting completions for find, none for head or sort, as per video above. Using native nvim LSP client.

@otreblan
Copy link
Contributor Author

Version 1. Works with @2, thank you! Although I am only getting completions for find, none for head or sort, as per video above. Using native nvim LSP client.

image

@gegoune
Copy link

gegoune commented Aug 11, 2021

I see, there is no completions for those in /usr/local/share/bash-completion/completions/. Commands with completions there do work fine. Great job, thanks!

@David-Else
Copy link
Contributor

Hi, I can't find any documentation on this feature in the README.

I have the following installed, and it works with find - [tab tab] on the command line:

Installed Packages
Name         : bash-completion
Epoch        : 1
Version      : 2.7
Release      : 5.el8
Architecture : noarch
Size         : 895 k
Source       : bash-completion-2.7-5.el8.src.rpm
Repository   : @System
From repo    : anaconda
Summary      : Programmable completion for Bash
URL          : https://github.com/scop/bash-completion
License      : GPLv2+
Description  : bash-completion is a collection of shell functions that take advantage
             : of the programmable completion feature of bash.

It does not seem to work in Neovim using the default config for the LSP https://github.com/neovim/nvim-lspconfig/blob/master/CONFIG.md#bashls and https://github.com/hrsh7th/nvim-cmp . Can anyone help? Thanks.

npm ls -g --depth=0
/usr/local/lib
├── bash-language-server@2.0.0

@otreblan
Copy link
Contributor Author

otreblan commented Sep 3, 2021

@David-Else Whats the output of pkg-config --variable=datadir bash-completion?

@David-Else
Copy link
Contributor

David-Else commented Sep 3, 2021

@otreblan The result is nothing, just an empty line. Also echo $BASH_COMPLETION gives an empty line too.

@otreblan
Copy link
Contributor Author

otreblan commented Sep 3, 2021

@David-Else Does your installation has /usr/share/pkgconfig/bash-completion.pc?

@David-Else
Copy link
Contributor

@otreblan yes:

$ls /usr/share/pkgconfig/
total 48K
drwxr-xr-x.   2 root root 4.0K Aug 16 11:42 .
drwxr-xr-x. 233 root root  12K Jul 21 09:17 ..
(various files)
-rw-r--r--.   1 root root  292 Apr 12 01:35 bash-completion.pc

@otreblan
Copy link
Contributor Author

otreblan commented Sep 3, 2021

@David-Else And it's contents?

@David-Else
Copy link
Contributor

@otreblan

bat /usr/share/pkgconfig/bash-completion.pc 
───────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: /usr/share/pkgconfig/bash-completion.pc
───────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ prefix=/usr
   2   │ compatdir=/etc/bash_completion.d
   3   │ completionsdir=${prefix}/share/bash-completion/completions
   4   │ helpersdir=${prefix}/share/bash-completion/helpers
   5   │ 
   6   │ Name: bash-completion
   7   │ Description: programmable completion for the bash shell
   8   │ URL: https://github.com/scop/bash-completion
   9   │ Version: 2.7
───────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Am I still meant to add autocmd FileType sh set iskeyword+=45, I assumed that was fixed somewhere in this PR?

@otreblan
Copy link
Contributor Author

otreblan commented Sep 3, 2021

@David-Else Your bash-completion is too old, datadir was added 2 years ago https://github.com/scop/bash-completion/blame/master/bash-completion.pc.in

@David-Else
Copy link
Contributor

@otreblan Cheers! I will try to update using the source from Github, I am using Rocky Linux 8, and they do have some out of date things hidden in there.

akurtakov added a commit to akurtakov/shellwax that referenced this pull request Sep 9, 2021
Noteworthy changes:
* Upgrade dependencies
* Adds support for completing command line arguments
(bash-lsp/bash-language-server#294)
* Default configuration change: parsing errors are not highlighted as
problems (as the grammar is buggy)
akurtakov added a commit to akurtakov/shellwax that referenced this pull request Sep 9, 2021
Noteworthy changes:
* Upgrade dependencies
* Adds support for completing command line arguments
(bash-lsp/bash-language-server#294)
* Default configuration change: parsing errors are not highlighted as
problems (as the grammar is buggy)
akurtakov added a commit to eclipse-shellwax/shellwax that referenced this pull request Sep 9, 2021
Noteworthy changes:
* Upgrade dependencies
* Adds support for completing command line arguments
(bash-lsp/bash-language-server#294)
* Default configuration change: parsing errors are not highlighted as
problems (as the grammar is buggy)
@David-Else
Copy link
Contributor

David-Else commented Feb 12, 2022

@otreblan Hello again. I removed my old Centos 8 RPM version of bash-completion and installed the latest from source. Now my bash-completion.pc is installed in /usr/local/share/pkgconfig and says:

prefix=/usr/local
datadir=/usr/local/share
sysconfdir=/usr/local/etc

compatdir=${sysconfdir}/bash_completion.d
completionsdir=${datadir}/bash-completion/completions
helpersdir=${datadir}/bash-completion/helpers

Name: bash-completion
Description: programmable completion for the bash shell
URL: https://github.com/scop/bash-completion
Version: 2.11

I am still not getting auto completion in neovim. You asked me before did I have /usr/share/pkgconfig/bash-completion.pc, now the file has moved could that be the problem? Thanks.

EDIT pkg-config --variable=datadir bash-completion still shows nothing

@David-Else
Copy link
Contributor

@otreblan Thanks to the new 2.1 version it now works! :)

@David-Else
Copy link
Contributor

This feature works in Helix, but there is a problem. It does not happen in Neovim, can it be solved here or should I make a request to the Helix devs? If so, what should I ask them to implement? Cheers!

  1. Type find - and ctrl-x to trigger autocomplete:
    Screenshot from 2023-01-04 17-27-58

  2. Select depth

Screenshot from 2023-01-04 17-28-01

  1. Now you are left with the original - that should not be there, it should be -depth not --depth

Screenshot from 2023-01-04 17-28-03

@skovhus
Copy link
Collaborator

skovhus commented Jan 4, 2023

@David-Else it seems like a Helix issue as it works in all other clients. And I'm honestly not sure what the issue is.

@David-Else
Copy link
Contributor

@skovhus Thanks for the quick reply!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate with bash-completion, if available
4 participants