Skip to content
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

Support format using shfmt (fixes #320) #1136

Merged
merged 13 commits into from
May 2, 2024

Conversation

chris-reeves
Copy link
Contributor

This PR adds support for formatting using shfmt (if installed). This implements enhancement #320.

Limited to using shfmt's defaults for now - no way to tweak these.
If the editor config is set to insert spaces instead of tabs, shfmt will
respect that choice (including the number of spaces to insert).
Add support for the following `shfmt` printer flags to control
formatting:
* `-bn`, `--binary-next-line`
* `-ci`, `--case-indent`
* `-fn`, `--func-next-line`
* `-sr`, `--space-redirects`

The `-i`, `--indent` flag is set appropriately based upon editor
formatting options.

Support for `-kp`, `--keep-padding` was not added as it has been
deprecated and will be removed in the next major release of `shfmt`.
The booleans should be booleans, not strings...
The deactivate function was throwing exceptions as the `client` var
didn't always have a value. Handle this by checking the var and
returning undefined if it doesn't exist.
@chris-reeves chris-reeves force-pushed the 320/format-using-shfmt branch from 7f71ff9 to b0dd1e3 Compare March 30, 2024 18:44
@chris-reeves
Copy link
Contributor Author

@skovhus Is there anything else you need me to do in order to get this looked at?

@skovhus
Copy link
Collaborator

skovhus commented Apr 30, 2024

Sorry, I completely missed this one.

@skovhus skovhus enabled auto-merge April 30, 2024 10:43
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.

This looks great! Thanks for contributing 👏

@skovhus skovhus disabled auto-merge April 30, 2024 10:43
@skovhus
Copy link
Collaborator

skovhus commented Apr 30, 2024

There are some CI errors here related to outdates snapshots. Can you have a look?

shfmt must be installed in order for the tests to pass in the GitHub
'verify' workflow.
@chris-reeves
Copy link
Contributor Author

chris-reeves commented Apr 30, 2024

There are some CI errors here related to outdates snapshots. Can you have a look?

shfmt needs to be installed in order for the tests to pass. I've updated the GitHub Actions workflow to install it. I haven't tested this, but it should work. If there are still issues with this run then I'll raise a pull request within my fork and debug.

@skovhus
Copy link
Collaborator

skovhus commented Apr 30, 2024

Still some issues here.

Long options are not supported in shfmt <= 3.5.0. Ubuntu 22.04 packages
shfmt 3.4.3.
@chris-reeves
Copy link
Contributor Author

The version of shfmt packaged on Ubuntu 22.04 is 3.4.3, which doesn't support long options. Switched to use short options now. This is a good catch, as there are likely lots of users running that version (or older).

All checks passed on my forked repo, so third time lucky...

Copy link

codecov bot commented May 2, 2024

Codecov Report

Attention: Patch coverage is 68.85246% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 81.25%. Comparing base (d272cb2) to head (9d5d784).
Report is 1 commits behind head on main.

Files Patch % Lines
server/src/server.ts 23.52% 11 Missing and 2 partials ⚠️
server/src/shfmt/index.ts 92.68% 3 Missing ⚠️
vscode-client/src/extension.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1136      +/-   ##
==========================================
- Coverage   81.52%   81.25%   -0.28%     
==========================================
  Files          28       29       +1     
  Lines        1348     1408      +60     
  Branches      320      334      +14     
==========================================
+ Hits         1099     1144      +45     
- Misses        204      217      +13     
- Partials       45       47       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@skovhus skovhus merged commit 1c2a204 into bash-lsp:main May 2, 2024
6 checks passed
@ambroisie
Copy link

Should it use -ln posix/-ln bash depending on whether the script is POSIX/bash?

Similarly, I believe the -s (simplify) option is missing from the configuration.

@chris-reeves
Copy link
Contributor Author

Should it use -ln posix/-ln bash depending on whether the script is POSIX/bash?

It's a month since I looked at this in detail. I did consider setting dialect, but shfmt's autodetection looked pretty good and setting a config option probably wouldn't be the right way for over-riding this on a per-script basis.

Similarly, I believe the -s (simplify) option is missing from the configuration.

Again, that was deliberate decision. The 'simplify' option doesn't just change formatting - it changes the code itself. That didn't seem like something a user would expect to happen when they format a document.

Having said that, if the community feels strongly that that option should be there it probably wouldn't be much effort for me (or someone else) to add it.

@David-Else
Copy link
Contributor

In Helix now with 5.3.1 when I save a ZSH file with auto-format on it deletes the contents of the file and saves an empty file. When I used shfmt by itself as an external formatter for Helix I did not have this problem: https://github.com/helix-editor/helix/wiki/External-formatter-configuration#shfmt

This might be an unrelated problem as I understand this language server is not compatible with ZSH, but it might be of interest. Helix shows that there are parsing errors, but it still more or less worked before enabling LSP formatting:

2024-05-14T11:19:37.486 helix_lsp::transport [INFO] bash-language-server <- {"jsonrpc":"2.0","method":"window/logMessage","params":{"type":2,"message":"10:19:37.485 WARNING ⛔️ Error while parsing file:///home/david/.zshrc: syntax error"}}
2024-05-14T11:19:37.486 helix_term::application [DEBUG] received editor event: LanguageServerMessage((LanguageServerId(1v1), Notification(Notification { jsonrpc: Some(V2), method: "window/logMessage", params: Map({"message": String("10:19:37.485 WARNING ⛔\u{fe0f} Error while parsing file:///home/david/.zshrc: syntax error"), "type": Number(2)}) })))
2024-05-14T11:19:37.486 helix_term::application [INFO] window/logMessage: LogMessageParams { typ: Warning, message: "10:19:37.485 WARNING ⛔\u{fe0f} Error while parsing file:///home/david/.zshrc: syntax error" }

@chris-reeves
Copy link
Contributor Author

In Helix now with 5.3.1 when I save a ZSH file with auto-format on it deletes the contents of the file and saves an empty file. When I used shfmt by itself as an external formatter for Helix I did not have this problem: https://github.com/helix-editor/helix/wiki/External-formatter-configuration#shfmt

This might be an unrelated problem as I understand this language server is not compatible with ZSH, but it might be of interest. Helix shows that there are parsing errors, but it still more or less worked before enabling LSP formatting:

2024-05-14T11:19:37.486 helix_lsp::transport [INFO] bash-language-server <- {"jsonrpc":"2.0","method":"window/logMessage","params":{"type":2,"message":"10:19:37.485 WARNING ⛔️ Error while parsing file:///home/david/.zshrc: syntax error"}}
2024-05-14T11:19:37.486 helix_term::application [DEBUG] received editor event: LanguageServerMessage((LanguageServerId(1v1), Notification(Notification { jsonrpc: Some(V2), method: "window/logMessage", params: Map({"message": String("10:19:37.485 WARNING ⛔\u{fe0f} Error while parsing file:///home/david/.zshrc: syntax error"), "type": Number(2)}) })))
2024-05-14T11:19:37.486 helix_term::application [INFO] window/logMessage: LogMessageParams { typ: Warning, message: "10:19:37.485 WARNING ⛔\u{fe0f} Error while parsing file:///home/david/.zshrc: syntax error" }

Could you share a (minimal) example file so that I can look into this from the shfmt/bash-language-server side please? Also, which version of shfmt are you running?

@David-Else
Copy link
Contributor

shfmt --version 3.6.0 on Debian 12.

Here is the file that when formatted goes blank. Just load it info Helix and use :format. Afraid I just changed everything back to the old setup so don't have time to go back and make this a minimal example file:

.zshrc

HISTFILE=~/.histfile
HISTSIZE=1000
SAVEHIST=1000
unsetopt autocd beep
bindkey -v
zstyle :compinstall filename "$HOME/.zshrc"
setopt PROMPT_SUBST
autoload -Uz vcs_info
zstyle ':vcs_info:git:*' formats '%F{#888888} (%F{#888888}󰘬 %b)%f'
precmd() { vcs_info }
PROMPT='%n@%m:%~${vcs_info_msg_0_}%(!.#.$) '

# use the shell's completion feature
autoload -Uz compinit
zstyle ':completion:*' menu select
zmodload zsh/complist
compinit

# Use vim keys in tab complete menu:
bindkey -M menuselect 'h' vi-backward-char
bindkey -M menuselect 'k' vi-up-line-or-history
bindkey -M menuselect 'l' vi-forward-char
bindkey -M menuselect 'j' vi-down-line-or-history

bindkey -M vicmd 'U' redo # use Helix redo shortcut

# Add to $PATH
PATH="$HOME/.deno/bin:$HOME/Documents/scripts:$HOME/.cargo/bin:$HOME/.local/bin:/usr/local/go/bin:$HOME/go/bin:$HOME/.local/kitty.app/bin:$PATH"
export PATH

# Aliases
alias ls="ls -ltha --color --group-directories-first --hyperlink=auto"
alias tree="tree -Catr --noreport --dirsfirst --filelimit 100"
alias ezrc='hx ~/.zshrc && source ~/.zshrc'
alias ai="aichat"
alias n="nnn"

# Functions
md() {
    filename="${1##*/}"
    pandoc --self-contained --metadata title="Preview" "$1" -o /tmp/"$filename".html
    xdg-open /tmp/"$filename".html
}

ghpr() { GH_FORCE_TTY=100% gh pr list --limit 300 |
    fzf --ansi --preview 'GH_FORCE_TTY=100% gh pr view {1}' --preview-window 'down,70%' --header-lines 3 |
    awk '{print $1}' |
    xargs gh pr checkout; }

wordcount() { pandoc --lua-filter wordcount.lua "$@"; }

# nnn
[ -n "$NNNLVL" ] && PS1="N$NNNLVL $PS1" # prompt you are within a shell that will return you to nnn
export NNN_PLUG='i:-!|mediainfo $nnn;d:dragdrop;f:fzcd;p:preview-tui;m:mtpmount;j:autojump'
export NNN_BMS="d:~/Documents;p:~/Pictures;v:~/Videos;m:~/Music;h:~/;u:/media/$USERNAME;D:~/Downloads;M:${XDG_CONFIG_HOME:-$HOME/.config}/nnn/mounts;a:/run/user/$UID/gvfs"
export NNN_TRASH=1 # use trash-cli: https://pypi.org/project/trash-cli/
export NNN_FIFO=/tmp/nnn.fifo
export NNN_BATTHEME="Visual Studio Dark+"
export NNN_BATSTYLE="plain"
export NNN_RCLONE='rclone mount --vfs-cache-mode writes'

# bat
export BAT_THEME="Visual Studio Dark+"
export MANPAGER="sh -c 'col -bx | batcat -l man -p'"

# fzf
export FZF_DEFAULT_COMMAND='rg --files --hidden --follow --no-ignore-vcs -g "!{node_modules,.git,.wine}"'
source /usr/share/doc/fzf/examples/key-bindings.zsh

# ytfzf
export video_pref="bestvideo[height<=?2160]+bestaudio/best"
alias ytfzf='ytfzf -T kitty'

# zoxide
eval "$(zoxide init zsh)"

stty -ixon # disable terminal flow control
export OPENAI_API_KEY="Add your key"
export EDITOR="hx"
export SUDO_EDITOR="$HOME/.cargo/bin/hx"

@chris-reeves
Copy link
Contributor Author

In Helix now with 5.3.1 when I save a ZSH file with auto-format on it deletes the contents of the file and saves an empty file. When I used shfmt by itself as an external formatter for Helix I did not have this problem: https://github.com/helix-editor/helix/wiki/External-formatter-configuration#shfmt

This might be an unrelated problem as I understand this language server is not compatible with ZSH, but it might be of interest. Helix shows that there are parsing errors, but it still more or less worked before enabling LSP formatting:

2024-05-14T11:19:37.486 helix_lsp::transport [INFO] bash-language-server <- {"jsonrpc":"2.0","method":"window/logMessage","params":{"type":2,"message":"10:19:37.485 WARNING ⛔️ Error while parsing file:///home/david/.zshrc: syntax error"}}
2024-05-14T11:19:37.486 helix_term::application [DEBUG] received editor event: LanguageServerMessage((LanguageServerId(1v1), Notification(Notification { jsonrpc: Some(V2), method: "window/logMessage", params: Map({"message": String("10:19:37.485 WARNING ⛔\u{fe0f} Error while parsing file:///home/david/.zshrc: syntax error"), "type": Number(2)}) })))
2024-05-14T11:19:37.486 helix_term::application [INFO] window/logMessage: LogMessageParams { typ: Warning, message: "10:19:37.485 WARNING ⛔\u{fe0f} Error while parsing file:///home/david/.zshrc: syntax error" }

Yes, this is a bug. I've opened issue #1162 to track this, and raised PR #1163 to fix.

@David-Else
Copy link
Contributor

@chris-reeves It is all fixed now, thanks for your prompt reaction!

chris-reeves added a commit to chris-reeves/bash-language-server that referenced this pull request Jun 1, 2024
This wasn't included initially as it does more than just format code.
However, its absence has been noted in a comment on the last PR:
bash-lsp#1136 (comment)
and it is a valid `shfmt` option, so support has been added.

There's a similar option (`-mn` or `--minify`), but this will not be
implemented as its output is not intended for human consumption and it
will essentially ignore all other options presented anyway.
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.

5 participants