-
Notifications
You must be signed in to change notification settings - Fork 137
Support format using shfmt (fixes #320) #1136
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
Conversation
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.
7f71ff9
to
b0dd1e3
Compare
@skovhus Is there anything else you need me to do in order to get this looked at?
Sorry, I completely missed this one.
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 looks great! Thanks for contributing 👏
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.
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.
Still some issues here.
Long options are not supported in shfmt <= 3.5.0. Ubuntu 22.04 packages shfmt 3.4.3.
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...
Codecov ReportAttention: Patch coverage is
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. |
ambroisie
commented
May 2, 2024
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.
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.
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/Formatter-Configurations#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月14日T11: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月14日T11: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月14日T11: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" }
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#shfmtThis 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月14日T11: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月14日T11: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月14日T11: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?
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"
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#shfmtThis 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月14日T11: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月14日T11: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月14日T11: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.
@chris-reeves It is all fixed now, thanks for your prompt reaction!
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.
This PR adds support for formatting using
shfmt
(if installed). This implements enhancement #320.