Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Complete command line arguments #294

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
skovhus merged 16 commits into bash-lsp:master from otreblan:master
Aug 11, 2021
Merged

Complete command line arguments #294

skovhus merged 16 commits into bash-lsp:master from otreblan:master
Aug 11, 2021

Conversation

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

ebkalderon, skovhus, and stephan49 reacted with hooray emoji
Copy link

codecov bot commented Jun 9, 2021
edited
Loading

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.

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.

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.

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

#!/usr/bin/env bash

source /usr/share/bash-completion/bash_completion
DATADIR="$(pkg-config --variable=datadir bash-completion)"
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.

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
edited
Loading

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

@otreblan otreblan Jul 4, 2021

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?

Copy link

gegoune commented Aug 11, 2021
edited
Loading

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).

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?

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.

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

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!

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

Copy link
Contributor Author

otreblan commented Sep 3, 2021

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

Copy link
Contributor

David-Else commented Sep 3, 2021
edited
Loading

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

Copy link
Contributor Author

otreblan commented Sep 3, 2021

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

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

Copy link
Contributor Author

otreblan commented Sep 3, 2021

@David-Else And it's contents?

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?

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

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)
Copy link
Contributor

David-Else commented Feb 12, 2022
edited
Loading

@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

Copy link
Contributor

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

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

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.

Copy link
Contributor

@skovhus Thanks for the quick reply!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skovhus skovhus skovhus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate with bash-completion, if available

AltStyle によって変換されたページ (->オリジナル) /