From 0c9504c371cd54539b18df143362289a0b2585e0 Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Mon, 3 Jun 2024 23:18:58 +0100 Subject: [PATCH 1/8] Add support for --language-dialect shfmt option Language dialect is something that can be set via `.editorconfig`, so we'll need to respect that when running `shfmt`. --- server/src/__tests__/config.test.ts | 6 ++++++ server/src/config.ts | 4 ++++ server/src/shfmt/__tests__/index.test.ts | 11 +++++++++++ server/src/shfmt/index.ts | 1 + vscode-client/package.json | 5 +++++ 5 files changed, 27 insertions(+) diff --git a/server/src/__tests__/config.test.ts b/server/src/__tests__/config.test.ts index ba927820f..dbe3a8151 100644 --- a/server/src/__tests__/config.test.ts +++ b/server/src/__tests__/config.test.ts @@ -18,6 +18,7 @@ describe('ConfigSchema', () => { "caseIndent": false, "funcNextLine": false, "keepPadding": false, + "languageDialect": "auto", "path": "shfmt", "simplifyCode": false, "spaceRedirects": false, @@ -39,6 +40,7 @@ describe('ConfigSchema', () => { caseIndent: true, funcNextLine: true, keepPadding: true, + languageDialect: 'posix', path: 'myshfmt', simplifyCode: true, spaceRedirects: true, @@ -64,6 +66,7 @@ describe('ConfigSchema', () => { "caseIndent": true, "funcNextLine": true, "keepPadding": true, + "languageDialect": "posix", "path": "myshfmt", "simplifyCode": true, "spaceRedirects": true, @@ -99,6 +102,7 @@ describe('getConfigFromEnvironmentVariables', () => { "caseIndent": false, "funcNextLine": false, "keepPadding": false, + "languageDialect": "auto", "path": "shfmt", "simplifyCode": false, "spaceRedirects": false, @@ -128,6 +132,7 @@ describe('getConfigFromEnvironmentVariables', () => { "caseIndent": false, "funcNextLine": false, "keepPadding": false, + "languageDialect": "auto", "path": "", "simplifyCode": false, "spaceRedirects": false, @@ -166,6 +171,7 @@ describe('getConfigFromEnvironmentVariables', () => { "caseIndent": true, "funcNextLine": false, "keepPadding": false, + "languageDialect": "auto", "path": "/path/to/shfmt", "simplifyCode": false, "spaceRedirects": false, diff --git a/server/src/config.ts b/server/src/config.ts index 26584cfa0..51398b689 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), + // Language dialect to use when parsing (bash/posix/mksh/bats). + languageDialect: z.string().trim().default('auto'), + // Simplify code before formatting. simplifyCode: z.boolean().default(false), @@ -84,6 +87,7 @@ export function getConfigFromEnvironmentVariables(): { shellcheckPath: process.env.SHELLCHECK_PATH, shfmt: { path: process.env.SHFMT_PATH, + languageDialect: process.env.SHFMT_LANGUAGE_DIALECT, binaryNextLine: toBoolean(process.env.SHFMT_BINARY_NEXT_LINE), caseIndent: toBoolean(process.env.SHFMT_CASE_INDENT), funcNextLine: toBoolean(process.env.SHFMT_FUNC_NEXT_LINE), diff --git a/server/src/shfmt/__tests__/index.test.ts b/server/src/shfmt/__tests__/index.test.ts index 481120a8c..fe578a091 100644 --- a/server/src/shfmt/__tests__/index.test.ts +++ b/server/src/shfmt/__tests__/index.test.ts @@ -62,6 +62,17 @@ describe('formatter', () => { ) }) + it('should throw when parsing using the wrong language dialect', async () => { + expect(async () => { + await getFormattingResult({ + document: FIXTURE_DOCUMENT.SHFMT, + shfmtConfig: { languageDialect: 'posix' }, + }) + }).rejects.toThrow( + /Shfmt: exited with status 1: .*\/testing\/fixtures\/shfmt.sh:25:14: the "function" builtin exists in bash; tried parsing as posix/, + ) + }) + it('should format when shfmt is present', async () => { const [result] = await getFormattingResult({ document: FIXTURE_DOCUMENT.SHFMT }) expect(result).toMatchInlineSnapshot(` diff --git a/server/src/shfmt/index.ts b/server/src/shfmt/index.ts index da3ea0a3d..bedd86278 100644 --- a/server/src/shfmt/index.ts +++ b/server/src/shfmt/index.ts @@ -71,6 +71,7 @@ export class Formatter { if (shfmtConfig?.keepPadding) args.push('-kp') // --keep-padding if (shfmtConfig?.simplifyCode) args.push('-s') // --simplify if (shfmtConfig?.spaceRedirects) args.push('-sr') // --space-redirects + if (shfmtConfig?.languageDialect) args.push(`-ln=${shfmtConfig.languageDialect}`) // --language-dialect // If we can determine a local filename, pass that to shfmt to aid language dialect detection const filePathMatch = document.uri.match(/^file:\/\/(.*)$/) diff --git a/vscode-client/package.json b/vscode-client/package.json index 457653cc9..1e563d7b7 100644 --- a/vscode-client/package.json +++ b/vscode-client/package.json @@ -83,6 +83,11 @@ "default": "shfmt", "description": "Controls the executable used for Shfmt formatting. An empty string will disable formatting." }, + "bashIde.shfmt.languageDialect": { + "type": "string", + "default": "auto", + "description": "Language dialect to use when parsing (bash/posix/mksh/bats)." + }, "bashIde.shfmt.binaryNextLine": { "type": "boolean", "default": false, From e038a25d32773fd49c4bd6ccc24e5eb095e64ea2 Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Mon, 3 Jun 2024 23:29:46 +0100 Subject: [PATCH 2/8] Use an enum for shfmt language dialect config --- server/src/config.ts | 6 +++--- vscode-client/package.json | 7 +++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/server/src/config.ts b/server/src/config.ts index 51398b689..076e22019 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -46,6 +46,9 @@ export const ConfigSchema = z.object({ // Controls the executable used for Shfmt formatting. An empty string will disable formatting path: z.string().trim().default('shfmt'), + // Language dialect to use when parsing (bash/posix/mksh/bats). + languageDialect: z.enum(['auto', 'bash', 'posix', 'mksh', 'bats']).default('auto'), + // Allow boolean operators (like && and ||) to start a line. binaryNextLine: z.boolean().default(false), @@ -58,9 +61,6 @@ export const ConfigSchema = z.object({ // (Deprecated) Keep column alignment padding. keepPadding: z.boolean().default(false), - // Language dialect to use when parsing (bash/posix/mksh/bats). - languageDialect: z.string().trim().default('auto'), - // Simplify code before formatting. simplifyCode: z.boolean().default(false), diff --git a/vscode-client/package.json b/vscode-client/package.json index 1e563d7b7..5866bc0e6 100644 --- a/vscode-client/package.json +++ b/vscode-client/package.json @@ -86,6 +86,13 @@ "bashIde.shfmt.languageDialect": { "type": "string", "default": "auto", + "enum": [ + "auto", + "bash", + "posix", + "mksh", + "bats" + ], "description": "Language dialect to use when parsing (bash/posix/mksh/bats)." }, "bashIde.shfmt.binaryNextLine": { From 0c7dd3cf63fb1608170d43916be0e816a4662cac Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Thu, 6 Jun 2024 00:14:26 +0100 Subject: [PATCH 3/8] Add support for reading shfmt config options from .editorconfig If we have any `shfmt`-specific options in `.editorconfig`, use the config in `.editorconfig` and ignore the language server config (this is similar to `shfmt`'s approach of using either `.editorconfig` or command line flags, but not both). Indentation always comes via the editor - if someone is using `.editorconfig` then the expectation is that they will have configured their editor's indentation in this way too. --- pnpm-lock.yaml | 35 ++++++++++++--- server/package.json | 1 + server/src/shfmt/index.ts | 89 ++++++++++++++++++++++++++++++++------- 3 files changed, 104 insertions(+), 21 deletions(-) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7d832c80d..e0906b6fa 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -62,6 +62,9 @@ importers: server: dependencies: + editorconfig: + specifier: 2.0.0 + version: 2.0.0 fast-glob: specifier: 3.3.2 version: 3.3.2 @@ -855,6 +858,10 @@ packages: '@nodelib/fs.scandir': 2.1.5 fastq: 1.15.0 + /@one-ini/wasm@0.1.1: + resolution: {integrity: sha512-XuySG1E38YScSJoMlqovLru4KTUNSjgVTIjyh7qMX6aNN5HY5Ct5LhRJdxO79JtTzKfzV/bnWpz+zquYrISsvw==} + dev: false + /@sinclair/typebox@0.27.8: resolution: {integrity: sha512-+Fj43pSMwJs4KRrH/938Uf+uAELIgVBmQzg/q1YG10djyfA3TnrU8N8XzqCh/okZdszqBQTZf96idMfE5lnwTA==} dev: true @@ -1335,7 +1342,6 @@ packages: /balanced-match@1.0.2: resolution: {integrity: sha512-3oSeUO0TMV67hN1AmbXsK4yaqU7tjiHlbxRDZOpH0KW9+CeX4bRAaX0Anxt0tx2MrpRpWwQaPwIlISEJhYU5Pw==} - dev: true /brace-expansion@1.1.11: resolution: {integrity: sha512-iCuPHDFgrHX7H2vEI/5xpz07zSHB00TpugqhmYtVmMO6518mCuRMoOYFldEBl0g187ufozdaHgWKcYFb61qGiA==} @@ -1348,7 +1354,6 @@ packages: resolution: {integrity: sha512-XnAIvQ8eM+kC6aULx6wuQiwVsnzsi9d3WxzV3FpWTGA19F621kwdbsAcFKXgKUHZWsy+mY6iL1sHTxWEFCytDA==} dependencies: balanced-match: 1.0.2 - dev: true /braces@3.0.2: resolution: {integrity: sha512-b8um+L1RzM3WDSzvhm6gIz1yfTbBt6YTlcEKAvsmqCZZFw46z626lVj9j1yEPW33H5H+lBQpZMP1k8l+78Ha0A==} @@ -1480,6 +1485,11 @@ packages: delayed-stream: 1.0.0 dev: true + /commander@11.1.0: + resolution: {integrity: sha512-yPVavfyCcRhmorC7rWlkHn15b4wDVgVmBA7kV4QVBsF7kv/9TKJAbAXVTxvTnwP8HHKjRCJDClKbciiYS7p0DQ==} + engines: {node: '>=16'} + dev: false + /concat-map@0.0.1: resolution: {integrity: sha512-/Srv4dswyQNBfohGpz9o6Yb3Gz3SrUDqBH5rTuhGR7ahtlbYKnVxw2bCFMRljaA7EXHaXZ8wsHdodFvbkhKmqg==} dev: true @@ -1583,6 +1593,17 @@ packages: resolution: {integrity: sha512-3VdM/SXBZX2omc9JF9nOPCtDaYQ67BGp5CoLpIQlO2KCAPETs8TcDHacF26jXadGbvUteZzRTeos2fhID5+ucQ==} dev: false + /editorconfig@2.0.0: + resolution: {integrity: sha512-s1NQ63WQ7RNXH6Efb2cwuyRlfpbtdZubvfNe4vCuoyGPewNPY7vah8JUSOFBiJ+jr99Qh8t0xKv0oITc1dclgw==} + engines: {node: '>=16'} + hasBin: true + dependencies: + '@one-ini/wasm': 0.1.1 + commander: 11.1.0 + minimatch: 9.0.2 + semver: 7.5.4 + dev: false + /electron-to-chromium@1.4.449: resolution: {integrity: sha512-TxLRpRUj/107ATefeP8VIUWNOv90xJxZZbCW/eIbSZQiuiFANCx2b7u+GbVc9X4gU+xnbvypNMYVM/WArE1DNQ==} dev: true @@ -2692,7 +2713,6 @@ packages: engines: {node: '>=10'} dependencies: yallist: 4.0.0 - dev: true /make-dir@3.1.0: resolution: {integrity: sha512-g3FeP20LNwhALb/6Cz6Dd4F2ngze0jz7tbzrD2wAV+o9FeNHe4rL+yK2md0J/fiSf1sa1ADhXqi5+oVwOM/eGw==} @@ -2749,6 +2769,13 @@ packages: brace-expansion: 1.1.11 dev: true + /minimatch@9.0.2: + resolution: {integrity: sha512-PZOT9g5v2ojiTL7r1xF6plNHLtOeTpSlDI007As2NlA2aYBMfVom17yqa6QzhmDP8QOhn7LjHTg7DFCVSSa6yg==} + engines: {node: '>=16 || 14 >=14.17'} + dependencies: + brace-expansion: 2.0.1 + dev: false + /minimatch@9.0.3: resolution: {integrity: sha512-RHiac9mvaRw0x3AYRgDC1CxAP7HTcNrrECeA8YYJeWnpo+2Q5CegtZjaotWTWxDG3UeGA1coE05iH1mPjT/2mg==} engines: {node: '>=16 || 14 >=14.17'} @@ -3029,7 +3056,6 @@ packages: hasBin: true dependencies: lru-cache: 6.0.0 - dev: true /shebang-command@2.0.0: resolution: {integrity: sha512-kHxr2zZpYtdmrN1qDjrrX/Z1rR1kG8Dx+gkpK1G4eXmvXswmcE1hTWBWYUzlraYw1/yZp6YuDY77YtvbN0dmDA==} @@ -3380,7 +3406,6 @@ packages: /yallist@4.0.0: resolution: {integrity: sha512-3wdGidZyq5PB084XLES5TpOSRA3wjXAlIWMhum2kRcv/41Sn2emQ0dycQW4uZXLejwKvg6EsvbdlVL+FYEct7A==} - dev: true /yargs-parser@21.1.1: resolution: {integrity: sha512-tVpsJW7DdjecAiFpbIB1e3qxIQsE6NoPc5/eTdrbbIC4h0LVsWhnoa3g+m2HclBIujHzsxZ4VJVA+GUuc2/LBw==} diff --git a/server/package.json b/server/package.json index f7f6541b9..7544d3e97 100644 --- a/server/package.json +++ b/server/package.json @@ -17,6 +17,7 @@ "node": ">=16" }, "dependencies": { + "editorconfig": "2.0.0", "fast-glob": "3.3.2", "fuzzy-search": "3.2.1", "node-fetch": "2.7.0", diff --git a/server/src/shfmt/index.ts b/server/src/shfmt/index.ts index bedd86278..42e9c50e8 100644 --- a/server/src/shfmt/index.ts +++ b/server/src/shfmt/index.ts @@ -1,6 +1,7 @@ import { spawn } from 'child_process' +import * as editorconfig from 'editorconfig' import * as LSP from 'vscode-languageserver/node' -import { TextDocument, TextEdit } from 'vscode-languageserver-textdocument' +import { DocumentUri, TextDocument, TextEdit } from 'vscode-languageserver-textdocument' import { logger } from '../util/logger' @@ -58,26 +59,82 @@ export class Formatter { ] } + private async getShfmtArguments( + documentUri: DocumentUri, + formatOptions?: LSP.FormattingOptions | null, + lspShfmtConfig?: Record | null, + ): Promise { + const args: string[] = [] + + // this is the config that we'll use to build args - default to language server config + let activeShfmtConfig = { ...lspShfmtConfig } + + // do we have a document stored on the local filesystem? + const filepathMatch = documentUri.match(/^file:\/\/(.*)$/) + if (filepathMatch) { + const filepath = filepathMatch[1] + args.push(`--filename=${filepathMatch[1]}`) + + const editorconfigProperties = await editorconfig.parse(filepath) + logger.debug( + `Shfmt: found .editorconfig properties: ${JSON.stringify( + editorconfigProperties, + )}`, + ) + + const editorconfigShfmtConfig: Record = {} + editorconfigShfmtConfig.binaryNextLine = editorconfigProperties.binary_next_line + editorconfigShfmtConfig.caseIndent = editorconfigProperties.switch_case_indent + editorconfigShfmtConfig.funcNextLine = editorconfigProperties.function_next_line + editorconfigShfmtConfig.keepPadding = editorconfigProperties.keep_padding + // --simplify is not supported via .editorconfig + editorconfigShfmtConfig.spaceRedirects = editorconfigProperties.space_redirects + editorconfigShfmtConfig.languageDialect = editorconfigProperties.shell_variant + + // if we have any shfmt-specific options in .editorconfig, use the config in .editorconfig and + // ignore the language server config (this is similar to shfmt's approach of using either + // .editorconfig or command line flags, but not both) + if ( + editorconfigShfmtConfig.binaryNextLine !== undefined || + editorconfigShfmtConfig.caseIndent !== undefined || + editorconfigShfmtConfig.funcNextLine !== undefined || + editorconfigShfmtConfig.keepPadding !== undefined || + editorconfigShfmtConfig.spaceRedirects !== undefined || + editorconfigShfmtConfig.languageDialect !== undefined + ) { + logger.debug( + 'Shfmt: detected shfmt properties in .editorconfig - ignoring language server shfmt config', + ) + activeShfmtConfig = { ...editorconfigShfmtConfig } + } else { + logger.debug( + 'Shfmt: no shfmt properties found in .editorconfig - using language server shfmt config', + ) + } + } + + // indentation always comes via the editor - if someone is using .editorconfig then the + // expectation is that they will have configured their editor's indentation in this way too + const indentation: number = formatOptions?.insertSpaces ? formatOptions.tabSize : 0 + args.push(`-i=${indentation}`) // --indent + + if (activeShfmtConfig?.binaryNextLine) args.push('-bn') // --binary-next-line + if (activeShfmtConfig?.caseIndent) args.push('-ci') // --case-indent + if (activeShfmtConfig?.funcNextLine) args.push('-fn') // --func-next-line + if (activeShfmtConfig?.keepPadding) args.push('-kp') // --keep-padding + if (activeShfmtConfig?.simplifyCode) args.push('-s') // --simplify + if (activeShfmtConfig?.spaceRedirects) args.push('-sr') // --space-redirects + if (activeShfmtConfig?.languageDialect) args.push(`-ln=${activeShfmtConfig.languageDialect}`) // --language-dialect + + return args + } + private async runShfmt( document: TextDocument, formatOptions?: LSP.FormattingOptions | null, shfmtConfig?: Record | null, ): Promise { - const indentation: number = formatOptions?.insertSpaces ? formatOptions.tabSize : 0 - const args: string[] = [`-i=${indentation}`] // --indent - 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?.simplifyCode) args.push('-s') // --simplify - if (shfmtConfig?.spaceRedirects) args.push('-sr') // --space-redirects - if (shfmtConfig?.languageDialect) args.push(`-ln=${shfmtConfig.languageDialect}`) // --language-dialect - - // 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]}`) - } + const args = await this.getShfmtArguments(document.uri, formatOptions, shfmtConfig) logger.debug(`Shfmt: running "${this.executablePath} ${args.join(' ')}"`) From 9c7f72335b5fc695211bf5d6db0ad1ece440a44d Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Fri, 7 Jun 2024 00:02:53 +0100 Subject: [PATCH 4/8] Add tests for reading shfmt config options from .editorconfig --- server/src/shfmt/__tests__/index.test.ts | 150 +++++++++++++++++- .../no-shfmt-properties/.editorconfig | 3 + .../shfmt-properties-false/.editorconfig | 6 + .../shfmt-properties/.editorconfig | 7 + 4 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 testing/fixtures/shfmt-editorconfig/no-shfmt-properties/.editorconfig create mode 100644 testing/fixtures/shfmt-editorconfig/shfmt-properties-false/.editorconfig create mode 100644 testing/fixtures/shfmt-editorconfig/shfmt-properties/.editorconfig diff --git a/server/src/shfmt/__tests__/index.test.ts b/server/src/shfmt/__tests__/index.test.ts index fe578a091..79dea1b4c 100644 --- a/server/src/shfmt/__tests__/index.test.ts +++ b/server/src/shfmt/__tests__/index.test.ts @@ -596,7 +596,7 @@ describe('formatter', () => { `) }) - it('should omit filename from the shfmt comment when it cannot be determined', async () => { + it('should omit filename from the shfmt command 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. @@ -613,4 +613,152 @@ describe('formatter', () => { /Shfmt: exited with status 1: :10:1: > must be followed by a word/, ) }) + + describe('getShfmtArguments()', () => { + const lspShfmtConfig = { + binaryNextLine: true, + funcNextLine: true, + simplifyCode: true, + } + const lspShfmtArgs = ['-bn', '-fn', '-s'] + const formatOptions = { tabSize: 2, insertSpaces: true } + + const formatter = new Formatter({ + executablePath: 'shfmt', + }) + + describe('when the document URI is not a filepath', () => { + let shfmtArgs: string[] + const filepath = `${FIXTURE_FOLDER}/shfmt.sh` + + beforeAll(async () => { + // @ts-expect-error Testing a private method + shfmtArgs = await formatter.getShfmtArguments( + `test://${filepath}`, + formatOptions, + lspShfmtConfig, + ) + }) + + it('should use language server config', async () => { + expect(shfmtArgs).toEqual(expect.arrayContaining(lspShfmtArgs)) + expect(shfmtArgs.length).toEqual(4) // indentation + }) + + it('should use indentation config from the editor', () => { + expect(shfmtArgs).toContain('-i=2') + }) + + it('should not include the filename argument', async () => { + expect(shfmtArgs).not.toContain(`--filename=${filepath}`) + }) + }) + + describe('when no .editorconfig exists', () => { + let shfmtArgs: string[] + const filepath = `${FIXTURE_FOLDER}/shfmt.sh` + + beforeAll(async () => { + // @ts-expect-error Testing a private method + shfmtArgs = await formatter.getShfmtArguments( + `file://${filepath}`, + formatOptions, + lspShfmtConfig, + ) + }) + + it('should use language server config', () => { + expect(shfmtArgs).toEqual(expect.arrayContaining(lspShfmtArgs)) + expect(shfmtArgs.length).toEqual(5) // indentation + filename + }) + + it('should use indentation config from the editor', () => { + expect(shfmtArgs).toContain('-i=2') + }) + + it('should include the filename argument', () => { + expect(shfmtArgs).toContain(`--filename=${filepath}`) + }) + }) + + describe('when an .editorconfig exists without shfmt options', () => { + let shfmtArgs: string[] + const filepath = `${FIXTURE_FOLDER}/shfmt-editorconfig/no-shfmt-properties/foo.sh` + + beforeAll(async () => { + // @ts-expect-error Testing a private method + shfmtArgs = await formatter.getShfmtArguments( + `file://${filepath}`, + formatOptions, + lspShfmtConfig, + ) + }) + + it('should use language server config', () => { + expect(shfmtArgs).toEqual(expect.arrayContaining(lspShfmtArgs)) + expect(shfmtArgs.length).toEqual(5) // indentation + filename + }) + + it('should use indentation config from the editor', () => { + expect(shfmtArgs).toContain('-i=2') + }) + + it('should include the filename argument', () => { + expect(shfmtArgs).toContain(`--filename=${filepath}`) + }) + }) + + describe('when an .editorconfig exists and contains only false shfmt options', () => { + let shfmtArgs: string[] + const filepath = `${FIXTURE_FOLDER}/shfmt-editorconfig/shfmt-properties-false/foo.sh` + + beforeAll(async () => { + // @ts-expect-error Testing a private method + shfmtArgs = await formatter.getShfmtArguments( + `file://${filepath}`, + formatOptions, + lspShfmtConfig, + ) + }) + + it('should use .editorconfig config (even though no options are enabled)', () => { + expect(shfmtArgs.length).toEqual(2) // indentation + filename + }) + + it('should use indentation config from the editor', () => { + expect(shfmtArgs).toContain('-i=2') + }) + + it('should include the filename argument', () => { + expect(shfmtArgs).toContain(`--filename=${filepath}`) + }) + }) + + describe('when an .editorconfig exists and contains one or more shfmt options', () => { + let shfmtArgs: string[] + const filepath = `${FIXTURE_FOLDER}/shfmt-editorconfig/shfmt-properties/foo.sh` + + beforeAll(async () => { + // @ts-expect-error Testing a private method + shfmtArgs = await formatter.getShfmtArguments( + `file://${filepath}`, + formatOptions, + lspShfmtConfig, + ) + }) + + it('should use .editorconfig config', () => { + expect(shfmtArgs).toEqual(expect.arrayContaining(['-ci', '-sr', "-ln='mksh'"])) + expect(shfmtArgs.length).toEqual(5) // indentation + filename + }) + + it('should use indentation config from the editor', () => { + expect(shfmtArgs).toContain('-i=2') + }) + + it('should include the filename argument', () => { + expect(shfmtArgs).toContain(`--filename=${filepath}`) + }) + }) + }) }) diff --git a/testing/fixtures/shfmt-editorconfig/no-shfmt-properties/.editorconfig b/testing/fixtures/shfmt-editorconfig/no-shfmt-properties/.editorconfig new file mode 100644 index 000000000..fbe3aecad --- /dev/null +++ b/testing/fixtures/shfmt-editorconfig/no-shfmt-properties/.editorconfig @@ -0,0 +1,3 @@ +[*] +indent_style = space +indent_size = 3 diff --git a/testing/fixtures/shfmt-editorconfig/shfmt-properties-false/.editorconfig b/testing/fixtures/shfmt-editorconfig/shfmt-properties-false/.editorconfig new file mode 100644 index 000000000..33a35262a --- /dev/null +++ b/testing/fixtures/shfmt-editorconfig/shfmt-properties-false/.editorconfig @@ -0,0 +1,6 @@ +[*] +indent_style = space +indent_size = 3 + +switch_case_indent = false +space_redirects = false diff --git a/testing/fixtures/shfmt-editorconfig/shfmt-properties/.editorconfig b/testing/fixtures/shfmt-editorconfig/shfmt-properties/.editorconfig new file mode 100644 index 000000000..0495f4fd9 --- /dev/null +++ b/testing/fixtures/shfmt-editorconfig/shfmt-properties/.editorconfig @@ -0,0 +1,7 @@ +[*] +indent_style = space +indent_size = 3 + +switch_case_indent = true +space_redirects = true +shell_variant = 'mksh' From 6e419dad6fb07bcf269101c8ffe0d47fd11b14db Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Fri, 7 Jun 2024 22:56:10 +0100 Subject: [PATCH 5/8] Add config option to ignore .editorconfig when running shfmt --- server/src/__tests__/config.test.ts | 6 +++ server/src/config.ts | 4 ++ server/src/shfmt/__tests__/index.test.ts | 27 ++++++++++ server/src/shfmt/index.ts | 69 +++++++++++++----------- vscode-client/package.json | 5 ++ 5 files changed, 80 insertions(+), 31 deletions(-) diff --git a/server/src/__tests__/config.test.ts b/server/src/__tests__/config.test.ts index dbe3a8151..edfeab43e 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, + "ignoreEditorconfig": false, "keepPadding": false, "languageDialect": "auto", "path": "shfmt", @@ -39,6 +40,7 @@ describe('ConfigSchema', () => { binaryNextLine: true, caseIndent: true, funcNextLine: true, + ignoreEditorconfig: true, keepPadding: true, languageDialect: 'posix', path: 'myshfmt', @@ -65,6 +67,7 @@ describe('ConfigSchema', () => { "binaryNextLine": true, "caseIndent": true, "funcNextLine": true, + "ignoreEditorconfig": true, "keepPadding": true, "languageDialect": "posix", "path": "myshfmt", @@ -101,6 +104,7 @@ describe('getConfigFromEnvironmentVariables', () => { "binaryNextLine": false, "caseIndent": false, "funcNextLine": false, + "ignoreEditorconfig": false, "keepPadding": false, "languageDialect": "auto", "path": "shfmt", @@ -131,6 +135,7 @@ describe('getConfigFromEnvironmentVariables', () => { "binaryNextLine": false, "caseIndent": false, "funcNextLine": false, + "ignoreEditorconfig": false, "keepPadding": false, "languageDialect": "auto", "path": "", @@ -170,6 +175,7 @@ describe('getConfigFromEnvironmentVariables', () => { "binaryNextLine": false, "caseIndent": true, "funcNextLine": false, + "ignoreEditorconfig": false, "keepPadding": false, "languageDialect": "auto", "path": "/path/to/shfmt", diff --git a/server/src/config.ts b/server/src/config.ts index 076e22019..b37eee151 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -46,6 +46,9 @@ export const ConfigSchema = z.object({ // Controls the executable used for Shfmt formatting. An empty string will disable formatting path: z.string().trim().default('shfmt'), + // Ignore shfmt config options in .editorconfig (always use language server config) + ignoreEditorconfig: z.boolean().default(false), + // Language dialect to use when parsing (bash/posix/mksh/bats). languageDialect: z.enum(['auto', 'bash', 'posix', 'mksh', 'bats']).default('auto'), @@ -87,6 +90,7 @@ export function getConfigFromEnvironmentVariables(): { shellcheckPath: process.env.SHELLCHECK_PATH, shfmt: { path: process.env.SHFMT_PATH, + ignoreEditorconfig: toBoolean(process.env.SHFMT_IGNORE_EDITORCONFIG), languageDialect: process.env.SHFMT_LANGUAGE_DIALECT, binaryNextLine: toBoolean(process.env.SHFMT_BINARY_NEXT_LINE), caseIndent: toBoolean(process.env.SHFMT_CASE_INDENT), diff --git a/server/src/shfmt/__tests__/index.test.ts b/server/src/shfmt/__tests__/index.test.ts index 79dea1b4c..3ddbe2af1 100644 --- a/server/src/shfmt/__tests__/index.test.ts +++ b/server/src/shfmt/__tests__/index.test.ts @@ -760,5 +760,32 @@ describe('formatter', () => { expect(shfmtArgs).toContain(`--filename=${filepath}`) }) }) + + describe('when an .editorconfig exists but ignoreEditorconfig is set', () => { + let shfmtArgs: string[] + const filepath = `${FIXTURE_FOLDER}/shfmt-editorconfig/shfmt-properties/foo.sh` + + beforeAll(async () => { + // @ts-expect-error Testing a private method + shfmtArgs = await formatter.getShfmtArguments( + `file://${filepath}`, + formatOptions, + { ...lspShfmtConfig, ignoreEditorconfig: true }, + ) + }) + + it('should use language server config', () => { + expect(shfmtArgs).toEqual(expect.arrayContaining(lspShfmtArgs)) + expect(shfmtArgs.length).toEqual(5) // indentation + filename + }) + + it('should use indentation config from the editor', () => { + expect(shfmtArgs).toContain('-i=2') + }) + + it('should include the filename argument', () => { + expect(shfmtArgs).toContain(`--filename=${filepath}`) + }) + }) }) }) diff --git a/server/src/shfmt/index.ts b/server/src/shfmt/index.ts index 42e9c50e8..a230d03c7 100644 --- a/server/src/shfmt/index.ts +++ b/server/src/shfmt/index.ts @@ -75,40 +75,46 @@ export class Formatter { const filepath = filepathMatch[1] args.push(`--filename=${filepathMatch[1]}`) - const editorconfigProperties = await editorconfig.parse(filepath) - logger.debug( - `Shfmt: found .editorconfig properties: ${JSON.stringify( - editorconfigProperties, - )}`, - ) - - const editorconfigShfmtConfig: Record = {} - editorconfigShfmtConfig.binaryNextLine = editorconfigProperties.binary_next_line - editorconfigShfmtConfig.caseIndent = editorconfigProperties.switch_case_indent - editorconfigShfmtConfig.funcNextLine = editorconfigProperties.function_next_line - editorconfigShfmtConfig.keepPadding = editorconfigProperties.keep_padding - // --simplify is not supported via .editorconfig - editorconfigShfmtConfig.spaceRedirects = editorconfigProperties.space_redirects - editorconfigShfmtConfig.languageDialect = editorconfigProperties.shell_variant - - // if we have any shfmt-specific options in .editorconfig, use the config in .editorconfig and - // ignore the language server config (this is similar to shfmt's approach of using either - // .editorconfig or command line flags, but not both) - if ( - editorconfigShfmtConfig.binaryNextLine !== undefined || - editorconfigShfmtConfig.caseIndent !== undefined || - editorconfigShfmtConfig.funcNextLine !== undefined || - editorconfigShfmtConfig.keepPadding !== undefined || - editorconfigShfmtConfig.spaceRedirects !== undefined || - editorconfigShfmtConfig.languageDialect !== undefined - ) { + if (!lspShfmtConfig?.ignoreEditorconfig) { + const editorconfigProperties = await editorconfig.parse(filepath) logger.debug( - 'Shfmt: detected shfmt properties in .editorconfig - ignoring language server shfmt config', + `Shfmt: found .editorconfig properties: ${JSON.stringify( + editorconfigProperties, + )}`, ) - activeShfmtConfig = { ...editorconfigShfmtConfig } + + const editorconfigShfmtConfig: Record = {} + editorconfigShfmtConfig.binaryNextLine = editorconfigProperties.binary_next_line + editorconfigShfmtConfig.caseIndent = editorconfigProperties.switch_case_indent + editorconfigShfmtConfig.funcNextLine = editorconfigProperties.function_next_line + editorconfigShfmtConfig.keepPadding = editorconfigProperties.keep_padding + // --simplify is not supported via .editorconfig + editorconfigShfmtConfig.spaceRedirects = editorconfigProperties.space_redirects + editorconfigShfmtConfig.languageDialect = editorconfigProperties.shell_variant + + // if we have any shfmt-specific options in .editorconfig, use the config in .editorconfig and + // ignore the language server config (this is similar to shfmt's approach of using either + // .editorconfig or command line flags, but not both) + if ( + editorconfigShfmtConfig.binaryNextLine !== undefined || + editorconfigShfmtConfig.caseIndent !== undefined || + editorconfigShfmtConfig.funcNextLine !== undefined || + editorconfigShfmtConfig.keepPadding !== undefined || + editorconfigShfmtConfig.spaceRedirects !== undefined || + editorconfigShfmtConfig.languageDialect !== undefined + ) { + logger.debug( + 'Shfmt: detected shfmt properties in .editorconfig - ignoring language server shfmt config', + ) + activeShfmtConfig = { ...editorconfigShfmtConfig } + } else { + logger.debug( + 'Shfmt: no shfmt properties found in .editorconfig - using language server shfmt config', + ) + } } else { logger.debug( - 'Shfmt: no shfmt properties found in .editorconfig - using language server shfmt config', + 'Shfmt: configured to ignore .editorconfig - using language server shfmt config', ) } } @@ -124,7 +130,8 @@ export class Formatter { if (activeShfmtConfig?.keepPadding) args.push('-kp') // --keep-padding if (activeShfmtConfig?.simplifyCode) args.push('-s') // --simplify if (activeShfmtConfig?.spaceRedirects) args.push('-sr') // --space-redirects - if (activeShfmtConfig?.languageDialect) args.push(`-ln=${activeShfmtConfig.languageDialect}`) // --language-dialect + if (activeShfmtConfig?.languageDialect) + args.push(`-ln=${activeShfmtConfig.languageDialect}`) // --language-dialect return args } diff --git a/vscode-client/package.json b/vscode-client/package.json index 5866bc0e6..7fe6dee45 100644 --- a/vscode-client/package.json +++ b/vscode-client/package.json @@ -83,6 +83,11 @@ "default": "shfmt", "description": "Controls the executable used for Shfmt formatting. An empty string will disable formatting." }, + "bashIde.shfmt.ignoreEditorconfig": { + "type": "boolean", + "default": false, + "description": "Ignore shfmt config options in .editorconfig (always use language server config)" + }, "bashIde.shfmt.languageDialect": { "type": "string", "default": "auto", From f5fa4b56c2b5ffd52e495661c41dcb13ed1601ef Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Fri, 7 Jun 2024 23:18:30 +0100 Subject: [PATCH 6/8] Update README regarding shfmt .editorconfig support --- README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/README.md b/README.md index 6af1d2777..1226e85d1 100644 --- a/README.md +++ b/README.md @@ -195,6 +195,15 @@ there is no `shfmt`-specific configuration variable for this. If your editor is two-space indents then that's what it will use. If you're using tabs for indentation then `shfmt` will use that. +The `shfmt` integration also supports configuration via `.editorconfig`. If any `shfmt`-specific +configuration properties are found in `.editorconfig` then the config in `.editorconfig` will be +used and the language server config will be ignored. This follows `shfmt`'s approach of using either +`.editorconfig` or command line flags, but not both. Note that only `shfmt`-specific configuration +properties are read from `.editorconfig` - indentation preferences are still provided by the editor, +so to format using the indentation specified in `.editorconfig` make sure your editor is also +configured to read `.editorconfig`. It is possible to disable `.editorconfig` support and always use +the language server config by setting the "Ignore Editorconfig" configuration variable. + ## Logging The minimum logging level for the server can be adjusted using the `BASH_IDE_LOG_LEVEL` environment variable From bbfa220d60ec17d83632f8bc9c832b12c01b0837 Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Sat, 8 Jun 2024 13:51:51 +0100 Subject: [PATCH 7/8] Fix shfmt test to support multiple shfmt versions The error message that we expect from one of our tests is different depending upon which version of shfmt is being run. Fix the test to accept both error messages. --- server/src/shfmt/__tests__/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/shfmt/__tests__/index.test.ts b/server/src/shfmt/__tests__/index.test.ts index 3ddbe2af1..46379cd34 100644 --- a/server/src/shfmt/__tests__/index.test.ts +++ b/server/src/shfmt/__tests__/index.test.ts @@ -69,7 +69,7 @@ describe('formatter', () => { shfmtConfig: { languageDialect: 'posix' }, }) }).rejects.toThrow( - /Shfmt: exited with status 1: .*\/testing\/fixtures\/shfmt.sh:25:14: the "function" builtin exists in bash; tried parsing as posix/, + /Shfmt: exited with status 1: .*\/testing\/fixtures\/shfmt\.sh:25:14: (the "function" builtin exists in bash; tried parsing as posix|a command can only contain words and redirects; encountered \()/, ) }) From 986548db4b5ae600a5c716d2f65bf6b35c7fabba Mon Sep 17 00:00:00 2001 From: Kenneth Skovhus Date: Mon, 10 Jun 2024 08:53:12 +0200 Subject: [PATCH 8/8] Bump server to 5.4.0 --- 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 762c3434f..d9a7414b8 100644 --- a/server/CHANGELOG.md +++ b/server/CHANGELOG.md @@ -1,5 +1,9 @@ # Bash Language Server +## 5.4.0 + +- Add .editorconfig support for shfmt https://github.com/bash-lsp/bash-language-server/pull/1171 + ## 5.3.4 - Add additonal shfmt formatting config options https://github.com/bash-lsp/bash-language-server/pull/1168 diff --git a/server/package.json b/server/package.json index 7544d3e97..60c69a26f 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.4", + "version": "5.4.0", "main": "./out/server.js", "typings": "./out/server.d.ts", "bin": {