From 847c09dd224ac866934ff6560cd0128893ab2c9b Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Thu, 30 May 2024 23:33:33 +0100 Subject: [PATCH 1/6] Update README to note how shfmt indentation is controlled This may preempt further questions similar to #1161. --- README.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index d52e8845..6af1d277 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,10 @@ To be implemented: ### Dependencies -As a dependency, we recommend that you first install shellcheck [shellcheck][shellcheck] to enable linting: https://github.com/koalaman/shellcheck#installing . If shellcheck is installed, bash-language-server will automatically call it to provide linting and code analysis each time the file is updated (with debounce time of 500ms). +As a dependency, we recommend that you first install [shellcheck][shellcheck] to enable linting: +https://github.com/koalaman/shellcheck#installing . If `shellcheck` is installed, +bash-language-server will automatically call it to provide linting and code analysis each time the +file is updated (with debounce time of 500ms). If you want your shell scripts to be formatted consistently, you can install [shfmt][shfmt]. If `shfmt` is installed then your documents will be formatted whenever you take the 'format document' @@ -185,6 +188,13 @@ Using the built-in `eglot` lsp mode: (bash-ts-mode . eglot-ensure)) ``` +## `shfmt` integration + +The indentation used by `shfmt` is whatever has been configured for the current editor session, so +there is no `shfmt`-specific configuration variable for this. If your editor is configured for +two-space indents then that's what it will use. If you're using tabs for indentation then `shfmt` +will use that. + ## Logging The minimum logging level for the server can be adjusted using the `BASH_IDE_LOG_LEVEL` environment variable From 09d80dd2b84b6b32f012b508ac7833f662daec6f Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Fri, 31 May 2024 00:18:00 +0100 Subject: [PATCH 2/6] Add support for --keep-padding shfmt option This wasn't included initially as it has been deprecated: https://github.com/mvdan/sh/issues/658 However, it has been mentioned in #1165 and is a valid option in the current version of `shfmt`, so support has been added (with a suitable deprecation warning). --- server/src/__tests__/config.test.ts | 6 ++ server/src/config.ts | 4 ++ server/src/shfmt/__tests__/index.test.ts | 87 +++++++++++++++++++++++- server/src/shfmt/index.ts | 1 + testing/fixtures/shfmt.sh | 4 ++ vscode-client/package.json | 5 ++ 6 files changed, 105 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/config.test.ts b/server/src/__tests__/config.test.ts index 1d02dcf0..be2f797a 100644 --- a/server/src/__tests__/config.test.ts +++ b/server/src/__tests__/config.test.ts @@ -17,6 +17,7 @@ describe('ConfigSchema', () => { "binaryNextLine": false, "caseIndent": false, "funcNextLine": false, + "keepPadding": false, "path": "shfmt", "spaceRedirects": false, }, @@ -36,6 +37,7 @@ describe('ConfigSchema', () => { binaryNextLine: true, caseIndent: true, funcNextLine: true, + keepPadding: true, path: 'myshfmt', spaceRedirects: true, }, @@ -59,6 +61,7 @@ describe('ConfigSchema', () => { "binaryNextLine": true, "caseIndent": true, "funcNextLine": true, + "keepPadding": true, "path": "myshfmt", "spaceRedirects": true, }, @@ -92,6 +95,7 @@ describe('getConfigFromEnvironmentVariables', () => { "binaryNextLine": false, "caseIndent": false, "funcNextLine": false, + "keepPadding": false, "path": "shfmt", "spaceRedirects": false, }, @@ -119,6 +123,7 @@ describe('getConfigFromEnvironmentVariables', () => { "binaryNextLine": false, "caseIndent": false, "funcNextLine": false, + "keepPadding": false, "path": "", "spaceRedirects": false, }, @@ -155,6 +160,7 @@ describe('getConfigFromEnvironmentVariables', () => { "binaryNextLine": false, "caseIndent": true, "funcNextLine": false, + "keepPadding": false, "path": "/path/to/shfmt", "spaceRedirects": false, }, diff --git a/server/src/config.ts b/server/src/config.ts index 1e39903b..2526ff96 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -55,6 +55,9 @@ export const ConfigSchema = z.object({ // Place function opening braces on a separate line. funcNextLine: z.boolean().default(false), + // (Deprecated) Keep column alignment padding. + keepPadding: z.boolean().default(false), + // Follow redirection operators with a space. spaceRedirects: z.boolean().default(false), }) @@ -81,6 +84,7 @@ export function getConfigFromEnvironmentVariables(): { binaryNextLine: toBoolean(process.env.SHFMT_BINARY_NEXT_LINE), caseIndent: toBoolean(process.env.SHFMT_CASE_INDENT), funcNextLine: toBoolean(process.env.SHFMT_FUNC_NEXT_LINE), + keepPadding: toBoolean(process.env.SHFMT_KEEP_PADDING), spaceRedirects: toBoolean(process.env.SHFMT_SPACE_REDIRECTS), }, } diff --git a/server/src/shfmt/__tests__/index.test.ts b/server/src/shfmt/__tests__/index.test.ts index d29f98d0..f6f685fd 100644 --- a/server/src/shfmt/__tests__/index.test.ts +++ b/server/src/shfmt/__tests__/index.test.ts @@ -83,6 +83,10 @@ describe('formatter', () => { ;; esac + echo one two three + echo four five six + echo seven eight nine + echo space redirects >/dev/null function next() { @@ -128,6 +132,10 @@ describe('formatter', () => { ;; esac + echo one two three + echo four five six + echo seven eight nine + echo space redirects >/dev/null function next() { @@ -173,6 +181,10 @@ describe('formatter', () => { ;; esac + echo one two three + echo four five six + echo seven eight nine + echo space redirects >/dev/null function next() { @@ -219,6 +231,10 @@ describe('formatter', () => { ;; esac + echo one two three + echo four five six + echo seven eight nine + echo space redirects >/dev/null function next() { @@ -265,6 +281,10 @@ describe('formatter', () => { ;; esac + echo one two three + echo four five six + echo seven eight nine + echo space redirects >/dev/null function next() { @@ -311,6 +331,10 @@ describe('formatter', () => { ;; esac + echo one two three + echo four five six + echo seven eight nine + echo space redirects >/dev/null function next() @@ -333,6 +357,56 @@ describe('formatter', () => { `) }) + it('should format with padding kept as-is when keepPadding is true', async () => { + const [result] = await getFormattingResult({ + document: FIXTURE_DOCUMENT.SHFMT, + formatOptions: { tabSize: 2, insertSpaces: true }, + shfmtConfig: { keepPadding: true }, + }) + expect(result).toMatchInlineSnapshot(` + [ + { + "newText": "#!/bin/bash + set -ueo pipefail + + if [ -z "$arg" ]; then + echo indent + fi + + echo binary && + echo next line + + case "$arg" in + a) + echo case indent + ;; + esac + + echo one two three + echo four five six + echo seven eight nine + + echo space redirects >/dev/null + + function next() { + echo line + } + ", + "range": { + "end": { + "character": 2147483647, + "line": 2147483647, + }, + "start": { + "character": 0, + "line": 0, + }, + }, + }, + ] + `) + }) + it('should format with redirect operators followed by a space when spaceRedirects is true', async () => { const [result] = await getFormattingResult({ document: FIXTURE_DOCUMENT.SHFMT, @@ -358,6 +432,10 @@ describe('formatter', () => { ;; esac + echo one two three + echo four five six + echo seven eight nine + echo space redirects > /dev/null function next() { @@ -387,6 +465,7 @@ describe('formatter', () => { binaryNextLine: true, caseIndent: true, funcNextLine: true, + keepPadding: true, spaceRedirects: true, }, }) @@ -401,7 +480,7 @@ describe('formatter', () => { fi echo binary \\ - && echo next line + && echo next line case "$arg" in a) @@ -409,10 +488,14 @@ describe('formatter', () => { ;; esac + echo one two three + echo four five six + echo seven eight nine + echo space redirects > /dev/null function next() - { + { echo line } ", diff --git a/server/src/shfmt/index.ts b/server/src/shfmt/index.ts index e039a04d..6ef66372 100644 --- a/server/src/shfmt/index.ts +++ b/server/src/shfmt/index.ts @@ -70,6 +70,7 @@ export class Formatter { if (shfmtConfig?.binaryNextLine) args.push('-bn') // --binary-next-line if (shfmtConfig?.caseIndent) args.push('-ci') // --case-indent if (shfmtConfig?.funcNextLine) args.push('-fn') // --func-next-line + if (shfmtConfig?.keepPadding) args.push('-kp') // --keep-padding if (shfmtConfig?.spaceRedirects) args.push('-sr') // --space-redirects logger.debug(`Shfmt: running "${this.executablePath} ${args.join(' ')}"`) diff --git a/testing/fixtures/shfmt.sh b/testing/fixtures/shfmt.sh index ff14200f..61344b52 100644 --- a/testing/fixtures/shfmt.sh +++ b/testing/fixtures/shfmt.sh @@ -14,6 +14,10 @@ echo case indent ;; esac +echo one two three +echo four five six +echo seven eight nine + echo space redirects> /dev/null function next(){ diff --git a/vscode-client/package.json b/vscode-client/package.json index 7c164ccf..93d8020e 100644 --- a/vscode-client/package.json +++ b/vscode-client/package.json @@ -98,6 +98,11 @@ "default": false, "description": "Place function opening braces on a separate line." }, + "bashIde.shfmt.keepPadding": { + "type": "boolean", + "default": false, + "description": "(Deprecated) Keep column alignment padding." + }, "bashIde.shfmt.spaceRedirects": { "type": "boolean", "default": false, From 351b194ed2e060a3186e14fec6c18f784559d9a0 Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Fri, 31 May 2024 22:42:51 +0100 Subject: [PATCH 3/6] Add link to shfmt "keep padding" deprecation notice --- vscode-client/package.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/vscode-client/package.json b/vscode-client/package.json index 93d8020e..9744bea4 100644 --- a/vscode-client/package.json +++ b/vscode-client/package.json @@ -101,7 +101,8 @@ "bashIde.shfmt.keepPadding": { "type": "boolean", "default": false, - "description": "(Deprecated) Keep column alignment padding." + "description": "(Deprecated) Keep column alignment padding.", + "markdownDescription": "**([Deprecated](https://github.com/mvdan/sh/issues/658))** Keep column alignment padding." }, "bashIde.shfmt.spaceRedirects": { "type": "boolean", From 2d8122855f7dddbfb73f58da0b9c9326ccbc6e63 Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Fri, 31 May 2024 23:40:06 +0100 Subject: [PATCH 4/6] Add support for --simplify shfmt option 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: https://github.com/bash-lsp/bash-language-server/pull/1136#issuecomment-2090804329 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. --- server/src/__tests__/config.test.ts | 6 ++ server/src/config.ts | 4 ++ server/src/shfmt/__tests__/index.test.ts | 71 ++++++++++++++++++++++++ server/src/shfmt/index.ts | 1 + testing/fixtures/shfmt.sh | 2 + vscode-client/package.json | 5 ++ 6 files changed, 89 insertions(+) diff --git a/server/src/__tests__/config.test.ts b/server/src/__tests__/config.test.ts index be2f797a..ba927820 100644 --- a/server/src/__tests__/config.test.ts +++ b/server/src/__tests__/config.test.ts @@ -19,6 +19,7 @@ describe('ConfigSchema', () => { "funcNextLine": false, "keepPadding": false, "path": "shfmt", + "simplifyCode": false, "spaceRedirects": false, }, } @@ -39,6 +40,7 @@ describe('ConfigSchema', () => { funcNextLine: true, keepPadding: true, path: 'myshfmt', + simplifyCode: true, spaceRedirects: true, }, }), @@ -63,6 +65,7 @@ describe('ConfigSchema', () => { "funcNextLine": true, "keepPadding": true, "path": "myshfmt", + "simplifyCode": true, "spaceRedirects": true, }, } @@ -97,6 +100,7 @@ describe('getConfigFromEnvironmentVariables', () => { "funcNextLine": false, "keepPadding": false, "path": "shfmt", + "simplifyCode": false, "spaceRedirects": false, }, } @@ -125,6 +129,7 @@ describe('getConfigFromEnvironmentVariables', () => { "funcNextLine": false, "keepPadding": false, "path": "", + "simplifyCode": false, "spaceRedirects": false, }, } @@ -162,6 +167,7 @@ describe('getConfigFromEnvironmentVariables', () => { "funcNextLine": false, "keepPadding": false, "path": "/path/to/shfmt", + "simplifyCode": false, "spaceRedirects": false, }, } diff --git a/server/src/config.ts b/server/src/config.ts index 2526ff96..26584cfa 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -58,6 +58,9 @@ export const ConfigSchema = z.object({ // (Deprecated) Keep column alignment padding. keepPadding: z.boolean().default(false), + // Simplify code before formatting. + simplifyCode: z.boolean().default(false), + // Follow redirection operators with a space. spaceRedirects: z.boolean().default(false), }) @@ -85,6 +88,7 @@ export function getConfigFromEnvironmentVariables(): { caseIndent: toBoolean(process.env.SHFMT_CASE_INDENT), funcNextLine: toBoolean(process.env.SHFMT_FUNC_NEXT_LINE), keepPadding: toBoolean(process.env.SHFMT_KEEP_PADDING), + simplifyCode: toBoolean(process.env.SHFMT_SIMPLIFY_CODE), spaceRedirects: toBoolean(process.env.SHFMT_SPACE_REDIRECTS), }, } diff --git a/server/src/shfmt/__tests__/index.test.ts b/server/src/shfmt/__tests__/index.test.ts index f6f685fd..0c05e5d1 100644 --- a/server/src/shfmt/__tests__/index.test.ts +++ b/server/src/shfmt/__tests__/index.test.ts @@ -87,6 +87,8 @@ describe('formatter', () => { echo four five six echo seven eight nine + [[ "$simplify" == "simplify" ]] + echo space redirects >/dev/null function next() { @@ -136,6 +138,8 @@ describe('formatter', () => { echo four five six echo seven eight nine + [[ "$simplify" == "simplify" ]] + echo space redirects >/dev/null function next() { @@ -185,6 +189,8 @@ describe('formatter', () => { echo four five six echo seven eight nine + [[ "$simplify" == "simplify" ]] + echo space redirects >/dev/null function next() { @@ -235,6 +241,8 @@ describe('formatter', () => { echo four five six echo seven eight nine + [[ "$simplify" == "simplify" ]] + echo space redirects >/dev/null function next() { @@ -285,6 +293,8 @@ describe('formatter', () => { echo four five six echo seven eight nine + [[ "$simplify" == "simplify" ]] + echo space redirects >/dev/null function next() { @@ -335,6 +345,8 @@ describe('formatter', () => { echo four five six echo seven eight nine + [[ "$simplify" == "simplify" ]] + echo space redirects >/dev/null function next() @@ -386,6 +398,60 @@ describe('formatter', () => { echo four five six echo seven eight nine + [[ "$simplify" == "simplify" ]] + + echo space redirects >/dev/null + + function next() { + echo line + } + ", + "range": { + "end": { + "character": 2147483647, + "line": 2147483647, + }, + "start": { + "character": 0, + "line": 0, + }, + }, + }, + ] + `) + }) + + it('should format after simplifying the code when simplifyCode is true', async () => { + const [result] = await getFormattingResult({ + document: FIXTURE_DOCUMENT.SHFMT, + formatOptions: { tabSize: 2, insertSpaces: true }, + shfmtConfig: { simplifyCode: true }, + }) + expect(result).toMatchInlineSnapshot(` + [ + { + "newText": "#!/bin/bash + set -ueo pipefail + + if [ -z "$arg" ]; then + echo indent + fi + + echo binary && + echo next line + + case "$arg" in + a) + echo case indent + ;; + esac + + echo one two three + echo four five six + echo seven eight nine + + [[ $simplify == "simplify" ]] + echo space redirects >/dev/null function next() { @@ -436,6 +502,8 @@ describe('formatter', () => { echo four five six echo seven eight nine + [[ "$simplify" == "simplify" ]] + echo space redirects > /dev/null function next() { @@ -466,6 +534,7 @@ describe('formatter', () => { caseIndent: true, funcNextLine: true, keepPadding: true, + simplifyCode: true, spaceRedirects: true, }, }) @@ -492,6 +561,8 @@ describe('formatter', () => { echo four five six echo seven eight nine + [[ $simplify == "simplify" ]] + echo space redirects > /dev/null function next() diff --git a/server/src/shfmt/index.ts b/server/src/shfmt/index.ts index 6ef66372..00304d18 100644 --- a/server/src/shfmt/index.ts +++ b/server/src/shfmt/index.ts @@ -71,6 +71,7 @@ export class Formatter { if (shfmtConfig?.caseIndent) args.push('-ci') // --case-indent if (shfmtConfig?.funcNextLine) args.push('-fn') // --func-next-line if (shfmtConfig?.keepPadding) args.push('-kp') // --keep-padding + if (shfmtConfig?.simplifyCode) args.push('-s') // --simplify if (shfmtConfig?.spaceRedirects) args.push('-sr') // --space-redirects logger.debug(`Shfmt: running "${this.executablePath} ${args.join(' ')}"`) diff --git a/testing/fixtures/shfmt.sh b/testing/fixtures/shfmt.sh index 61344b52..0ab419a7 100644 --- a/testing/fixtures/shfmt.sh +++ b/testing/fixtures/shfmt.sh @@ -18,6 +18,8 @@ echo one two three echo four five six echo seven eight nine +[[ "$simplify" == "simplify" ]] + echo space redirects> /dev/null function next(){ diff --git a/vscode-client/package.json b/vscode-client/package.json index 9744bea4..457653cc 100644 --- a/vscode-client/package.json +++ b/vscode-client/package.json @@ -104,6 +104,11 @@ "description": "(Deprecated) Keep column alignment padding.", "markdownDescription": "**([Deprecated](https://github.com/mvdan/sh/issues/658))** Keep column alignment padding." }, + "bashIde.shfmt.simplifyCode": { + "type": "boolean", + "default": false, + "description": "Simplify code before formatting." + }, "bashIde.shfmt.spaceRedirects": { "type": "boolean", "default": false, From 773a61c211edc716c2888989e291d2ae7f712640 Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Sat, 1 Jun 2024 01:21:57 +0100 Subject: [PATCH 5/6] Pass --filename to shfmt when available This will aid in language dialect detection. --- server/src/shfmt/__tests__/index.test.ts | 20 +++++++++++++++++++- server/src/shfmt/index.ts | 14 +++++++++----- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/server/src/shfmt/__tests__/index.test.ts b/server/src/shfmt/__tests__/index.test.ts index 0c05e5d1..481120a8 100644 --- a/server/src/shfmt/__tests__/index.test.ts +++ b/server/src/shfmt/__tests__/index.test.ts @@ -58,7 +58,7 @@ describe('formatter', () => { expect(async () => { await getFormattingResult({ document: FIXTURE_DOCUMENT.PARSE_PROBLEMS }) }).rejects.toThrow( - 'Shfmt: exited with status 1: :10:1: > must be followed by a word', + /Shfmt: exited with status 1: .*\/testing\/fixtures\/parse-problems.sh:10:1: > must be followed by a word/, ) }) @@ -584,4 +584,22 @@ describe('formatter', () => { ] `) }) + + it('should omit filename from the shfmt comment when it cannot be determined', async () => { + // There's no easy way to see what filename has been passed to shfmt without inspecting the + // contents of the logs. As a workaround, we set a non-file:// URI on a dodgy document to + // trigger an exception and inspect the error message. + const testDocument = TextDocument.create( + 'http://localhost/', + 'shellscript', + 0, + FIXTURE_DOCUMENT.PARSE_PROBLEMS.getText(), + ) + + expect(async () => { + await getFormattingResult({ document: testDocument }) + }).rejects.toThrow( + /Shfmt: exited with status 1: :10:1: > must be followed by a word/, + ) + }) }) diff --git a/server/src/shfmt/index.ts b/server/src/shfmt/index.ts index 00304d18..da3ea0a3 100644 --- a/server/src/shfmt/index.ts +++ b/server/src/shfmt/index.ts @@ -41,9 +41,7 @@ export class Formatter { formatOptions?: LSP.FormattingOptions | null, shfmtConfig?: Record | null, ): Promise { - const documentText = document.getText() - - const result = await this.runShfmt(documentText, formatOptions, shfmtConfig) + const result = await this.runShfmt(document, formatOptions, shfmtConfig) if (!this._canFormat) { return [] @@ -61,7 +59,7 @@ export class Formatter { } private async runShfmt( - documentText: string, + document: TextDocument, formatOptions?: LSP.FormattingOptions | null, shfmtConfig?: Record | null, ): Promise { @@ -74,6 +72,12 @@ export class Formatter { if (shfmtConfig?.simplifyCode) args.push('-s') // --simplify if (shfmtConfig?.spaceRedirects) args.push('-sr') // --space-redirects + // If we can determine a local filename, pass that to shfmt to aid language dialect detection + const filePathMatch = document.uri.match(/^file:\/\/(.*)$/) + if (filePathMatch) { + args.push(`--filename=${filePathMatch[1]}`) + } + logger.debug(`Shfmt: running "${this.executablePath} ${args.join(' ')}"`) let out = '' @@ -90,7 +94,7 @@ export class Formatter { // This is solved in Node >= 15.1 by the "on('spawn', ...)" event, but we need to // support earlier versions. }) - proc.stdin.end(documentText) + proc.stdin.end(document.getText()) }) let exit From 3a5ac07cb2e2ba408844a1514d1058d298d6f0a4 Mon Sep 17 00:00:00 2001 From: Kenneth Skovhus Date: Sun, 2 Jun 2024 22:07:16 +0200 Subject: [PATCH 6/6] Bump server version 5.3.4 --- server/CHANGELOG.md | 4 ++++ server/package.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/server/CHANGELOG.md b/server/CHANGELOG.md index 52f62060..762c3434 100644 --- a/server/CHANGELOG.md +++ b/server/CHANGELOG.md @@ -1,5 +1,9 @@ # Bash Language Server +## 5.3.4 + +- Add additonal shfmt formatting config options https://github.com/bash-lsp/bash-language-server/pull/1168 + ## 5.3.3 - Revert "Add --help fallback for documentation" https://github.com/bash-lsp/bash-language-server/pull/1052 diff --git a/server/package.json b/server/package.json index 73df323e..f7f6541b 100644 --- a/server/package.json +++ b/server/package.json @@ -3,7 +3,7 @@ "description": "A language server for Bash", "author": "Mads Hartmann", "license": "MIT", - "version": "5.3.3", + "version": "5.3.4", "main": "./out/server.js", "typings": "./out/server.d.ts", "bin": {