Explorar o código

refactor: make formatter config opt-in (#22997)

Dax hai 1 día
pai
achega
220e3e9a2b

+ 1 - 1
packages/opencode/src/config/formatter.ts

@@ -9,5 +9,5 @@ export const Entry = z.object({
   extensions: z.array(z.string()).optional(),
 })
 
-export const Info = z.union([z.literal(false), z.record(z.string(), Entry)])
+export const Info = z.union([z.boolean(), z.record(z.string(), Entry)])
 export type Info = z.infer<typeof Info>

+ 1 - 1
packages/opencode/src/config/lsp.ts

@@ -18,7 +18,7 @@ export const Entry = z.union([
   }),
 ])
 
-export const Info = z.union([z.literal(false), z.record(z.string(), Entry)]).refine(
+export const Info = z.union([z.boolean(), z.record(z.string(), Entry)]).refine(
   (data) => {
     if (typeof data === "boolean") return true
     const serverIds = new Set(Object.values(LSPServer).map((server) => server.id))

+ 42 - 33
packages/opencode/src/format/index.ts

@@ -41,39 +41,6 @@ export const layer = Layer.effect(
         const commands: Record<string, string[] | false> = {}
         const formatters: Record<string, Formatter.Info> = {}
 
-        const cfg = yield* config.get()
-
-        if (cfg.formatter !== false) {
-          for (const item of Object.values(Formatter)) {
-            formatters[item.name] = item
-          }
-          for (const [name, item] of Object.entries(cfg.formatter ?? {})) {
-            // Ruff and uv are both the same formatter, so disabling either should disable both.
-            if (["ruff", "uv"].includes(name) && (cfg.formatter?.ruff?.disabled || cfg.formatter?.uv?.disabled)) {
-              // TODO combine formatters so shared backends like Ruff/uv don't need linked disable handling here.
-              delete formatters.ruff
-              delete formatters.uv
-              continue
-            }
-            if (item.disabled) {
-              delete formatters[name]
-              continue
-            }
-            const info = mergeDeep(formatters[name] ?? {}, {
-              extensions: [],
-              ...item,
-            })
-
-            formatters[name] = {
-              ...info,
-              name,
-              enabled: async () => info.command ?? false,
-            }
-          }
-        } else {
-          log.info("all formatters are disabled")
-        }
-
         async function getCommand(item: Formatter.Info) {
           let cmd = commands[item.name]
           if (cmd === false || cmd === undefined) {
@@ -149,6 +116,48 @@ export const layer = Layer.effect(
           })
         }
 
+        const cfg = yield* config.get()
+
+        if (!cfg.formatter) {
+          log.info("all formatters are disabled")
+          log.info("init")
+          return {
+            formatters,
+            isEnabled,
+            formatFile,
+          }
+        }
+
+        for (const item of Object.values(Formatter)) {
+          formatters[item.name] = item
+        }
+
+        if (cfg.formatter !== true) {
+          for (const [name, item] of Object.entries(cfg.formatter)) {
+            const builtIn = Formatter[name as keyof typeof Formatter]
+
+            // Ruff and uv are both the same formatter, so disabling either should disable both.
+            if (["ruff", "uv"].includes(name) && (cfg.formatter.ruff?.disabled || cfg.formatter.uv?.disabled)) {
+              // TODO combine formatters so shared backends like Ruff/uv don't need linked disable handling here.
+              delete formatters.ruff
+              delete formatters.uv
+              continue
+            }
+            if (item.disabled) {
+              delete formatters[name]
+              continue
+            }
+            const info = mergeDeep(builtIn ?? { extensions: [] }, item)
+
+            formatters[name] = {
+              ...info,
+              name,
+              extensions: info.extensions ?? [],
+              enabled: builtIn && !info.command ? builtIn.enabled : async () => info.command ?? false,
+            }
+          }
+        }
+
         log.info("init")
 
         return {

+ 21 - 19
packages/opencode/src/lsp/lsp.ts

@@ -167,7 +167,7 @@ export const layer = Layer.effect(
 
         const servers: Record<string, LSPServer.Info> = {}
 
-        if (cfg.lsp === false) {
+        if (!cfg.lsp) {
           log.info("all LSPs are disabled")
         } else {
           for (const server of Object.values(LSPServer)) {
@@ -176,25 +176,27 @@ export const layer = Layer.effect(
 
           filterExperimentalServers(servers)
 
-          for (const [name, item] of Object.entries(cfg.lsp ?? {})) {
-            const existing = servers[name]
-            if (item.disabled) {
-              log.info(`LSP server ${name} is disabled`)
-              delete servers[name]
-              continue
-            }
-            servers[name] = {
-              ...existing,
-              id: name,
-              root: existing?.root ?? (async () => Instance.directory),
-              extensions: item.extensions ?? existing?.extensions ?? [],
-              spawn: async (root) => ({
-                process: lspspawn(item.command[0], item.command.slice(1), {
-                  cwd: root,
-                  env: { ...process.env, ...item.env },
+          if (cfg.lsp !== true) {
+            for (const [name, item] of Object.entries(cfg.lsp)) {
+              const existing = servers[name]
+              if (item.disabled) {
+                log.info(`LSP server ${name} is disabled`)
+                delete servers[name]
+                continue
+              }
+              servers[name] = {
+                ...existing,
+                id: name,
+                root: existing?.root ?? (async () => Instance.directory),
+                extensions: item.extensions ?? existing?.extensions ?? [],
+                spawn: async (root) => ({
+                  process: lspspawn(item.command[0], item.command.slice(1), {
+                    cwd: root,
+                    env: { ...process.env, ...item.env },
+                  }),
+                  initialization: item.initialization,
                 }),
-                initialization: item.initialization,
-              }),
+              }
             }
           }
 

+ 36 - 0
packages/opencode/test/config/config.test.ts

@@ -142,6 +142,42 @@ test("loads JSON config file", async () => {
   })
 })
 
+test("loads formatter boolean config", async () => {
+  await using tmp = await tmpdir({
+    init: async (dir) => {
+      await writeConfig(dir, {
+        $schema: "https://opencode.ai/config.json",
+        formatter: true,
+      })
+    },
+  })
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      const config = await load()
+      expect(config.formatter).toBe(true)
+    },
+  })
+})
+
+test("loads lsp boolean config", async () => {
+  await using tmp = await tmpdir({
+    init: async (dir) => {
+      await writeConfig(dir, {
+        $schema: "https://opencode.ai/config.json",
+        lsp: true,
+      })
+    },
+  })
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      const config = await load()
+      expect(config.lsp).toBe(true)
+    },
+  })
+})
+
 test("loads project config from Git Bash and MSYS2 paths on Windows", async () => {
   // Git Bash and MSYS2 both use /<drive>/... paths on Windows.
   await check((dir) => {

+ 106 - 70
packages/opencode/test/format/format.test.ts

@@ -10,37 +10,55 @@ import * as Formatter from "../../src/format/formatter"
 const it = testEffect(Layer.mergeAll(Format.defaultLayer, CrossSpawnSpawner.defaultLayer, NodeFileSystem.layer))
 
 describe("Format", () => {
-  it.live("status() returns built-in formatters when no config overrides", () =>
+  it.live("status() returns empty list when no formatters are configured", () =>
     provideTmpdirInstance(() =>
       Format.Service.use((fmt) =>
         Effect.gen(function* () {
-          const statuses = yield* fmt.status()
-          expect(Array.isArray(statuses)).toBe(true)
-          expect(statuses.length).toBeGreaterThan(0)
-
-          for (const item of statuses) {
-            expect(typeof item.name).toBe("string")
-            expect(Array.isArray(item.extensions)).toBe(true)
-            expect(typeof item.enabled).toBe("boolean")
-          }
-
-          const gofmt = statuses.find((item) => item.name === "gofmt")
-          expect(gofmt).toBeDefined()
-          expect(gofmt!.extensions).toContain(".go")
+          expect(yield* fmt.status()).toEqual([])
         }),
       ),
     ),
   )
 
-  it.live("status() returns empty list when formatter is disabled", () =>
+  it.live("status() returns built-in formatters when formatter is true", () =>
     provideTmpdirInstance(
       () =>
         Format.Service.use((fmt) =>
           Effect.gen(function* () {
-            expect(yield* fmt.status()).toEqual([])
+            const statuses = yield* fmt.status()
+            const gofmt = statuses.find((item) => item.name === "gofmt")
+            expect(gofmt).toBeDefined()
+            expect(gofmt!.extensions).toContain(".go")
           }),
         ),
-      { config: { formatter: false } },
+      {
+        config: {
+          formatter: true,
+        },
+      },
+    ),
+  )
+
+  it.live("status() keeps built-in formatters when config object is provided", () =>
+    provideTmpdirInstance(
+      () =>
+        Format.Service.use((fmt) =>
+          Effect.gen(function* () {
+            const statuses = yield* fmt.status()
+            const gofmt = statuses.find((item) => item.name === "gofmt")
+            const mix = statuses.find((item) => item.name === "mix")
+            expect(gofmt).toBeDefined()
+            expect(gofmt!.extensions).toContain(".go")
+            expect(mix).toBeDefined()
+          }),
+        ),
+      {
+        config: {
+          formatter: {
+            gofmt: {},
+          },
+        },
+      },
     ),
   )
 
@@ -51,7 +69,9 @@ describe("Format", () => {
           Effect.gen(function* () {
             const statuses = yield* fmt.status()
             const gofmt = statuses.find((item) => item.name === "gofmt")
+            const mix = statuses.find((item) => item.name === "mix")
             expect(gofmt).toBeUndefined()
+            expect(mix).toBeDefined()
           }),
         ),
       {
@@ -111,68 +131,84 @@ describe("Format", () => {
       const a = yield* provideTmpdirInstance(() => Format.Service.use((fmt) => fmt.status()), {
         config: { formatter: false },
       })
-      const b = yield* provideTmpdirInstance(() => Format.Service.use((fmt) => fmt.status()))
+      const b = yield* provideTmpdirInstance(
+        () => Format.Service.use((fmt) => fmt.status()),
+        {
+          config: {
+            formatter: true,
+          },
+        },
+      )
 
       expect(a).toEqual([])
-      expect(b.length).toBeGreaterThan(0)
+      expect(b.find((item) => item.name === "gofmt")).toBeDefined()
     }),
   )
 
   it.live("runs enabled checks for matching formatters in parallel", () =>
-    provideTmpdirInstance((path) =>
-      Effect.gen(function* () {
-        const file = `${path}/test.parallel`
-        yield* Effect.promise(() => Bun.write(file, "x"))
-
-        const one = {
-          extensions: Formatter.gofmt.extensions,
-          enabled: Formatter.gofmt.enabled,
-        }
-        const two = {
-          extensions: Formatter.mix.extensions,
-          enabled: Formatter.mix.enabled,
-        }
-
-        let active = 0
-        let max = 0
-
-        yield* Effect.acquireUseRelease(
-          Effect.sync(() => {
-            Formatter.gofmt.extensions = [".parallel"]
-            Formatter.mix.extensions = [".parallel"]
-            Formatter.gofmt.enabled = async () => {
-              active++
-              max = Math.max(max, active)
-              await Bun.sleep(20)
-              active--
-              return ["sh", "-c", "true"]
-            }
-            Formatter.mix.enabled = async () => {
-              active++
-              max = Math.max(max, active)
-              await Bun.sleep(20)
-              active--
-              return ["sh", "-c", "true"]
-            }
-          }),
-          () =>
-            Format.Service.use((fmt) =>
-              Effect.gen(function* () {
-                yield* fmt.init()
-                yield* fmt.file(file)
-              }),
-            ),
-          () =>
+    provideTmpdirInstance(
+      (path) =>
+        Effect.gen(function* () {
+          const file = `${path}/test.parallel`
+          yield* Effect.promise(() => Bun.write(file, "x"))
+
+          const one = {
+            extensions: Formatter.gofmt.extensions,
+            enabled: Formatter.gofmt.enabled,
+          }
+          const two = {
+            extensions: Formatter.mix.extensions,
+            enabled: Formatter.mix.enabled,
+          }
+
+          let active = 0
+          let max = 0
+
+          yield* Effect.acquireUseRelease(
             Effect.sync(() => {
-              Formatter.gofmt.extensions = one.extensions
-              Formatter.gofmt.enabled = one.enabled
-              Formatter.mix.extensions = two.extensions
-              Formatter.mix.enabled = two.enabled
+              Formatter.gofmt.extensions = [".parallel"]
+              Formatter.mix.extensions = [".parallel"]
+              Formatter.gofmt.enabled = async () => {
+                active++
+                max = Math.max(max, active)
+                await Bun.sleep(20)
+                active--
+                return ["sh", "-c", "true"]
+              }
+              Formatter.mix.enabled = async () => {
+                active++
+                max = Math.max(max, active)
+                await Bun.sleep(20)
+                active--
+                return ["sh", "-c", "true"]
+              }
             }),
-        )
+            () =>
+              Format.Service.use((fmt) =>
+                Effect.gen(function* () {
+                  yield* fmt.init()
+                  yield* fmt.file(file)
+                }),
+              ),
+            () =>
+              Effect.sync(() => {
+                Formatter.gofmt.extensions = one.extensions
+                Formatter.gofmt.enabled = one.enabled
+                Formatter.mix.extensions = two.extensions
+                Formatter.mix.enabled = two.enabled
+              }),
+          )
 
-        expect(max).toBe(2)
-      }),
+          expect(max).toBe(2)
+        }),
+      {
+        config: {
+          formatter: {
+            gofmt: {},
+            mix: {},
+          },
+        },
+      },
     ),
   )
 

+ 73 - 19
packages/opencode/test/lsp/index.test.ts

@@ -11,28 +11,30 @@ const it = testEffect(Layer.mergeAll(LSP.defaultLayer, CrossSpawnSpawner.default
 
 describe("lsp.spawn", () => {
   it.live("does not spawn builtin LSP for files outside instance", () =>
-    provideTmpdirInstance((dir) =>
-      LSP.Service.use((lsp) =>
-        Effect.gen(function* () {
-          const spy = spyOn(LSPServer.Typescript, "spawn").mockResolvedValue(undefined)
+    provideTmpdirInstance(
+      (dir) =>
+        LSP.Service.use((lsp) =>
+          Effect.gen(function* () {
+            const spy = spyOn(LSPServer.Typescript, "spawn").mockResolvedValue(undefined)
 
-          try {
-            yield* lsp.touchFile(path.join(dir, "..", "outside.ts"))
-            yield* lsp.hover({
-              file: path.join(dir, "..", "hover.ts"),
-              line: 0,
-              character: 0,
-            })
-            expect(spy).toHaveBeenCalledTimes(0)
-          } finally {
-            spy.mockRestore()
-          }
-        }),
-      ),
+            try {
+              yield* lsp.touchFile(path.join(dir, "..", "outside.ts"))
+              yield* lsp.hover({
+                file: path.join(dir, "..", "hover.ts"),
+                line: 0,
+                character: 0,
+              })
+              expect(spy).toHaveBeenCalledTimes(0)
+            } finally {
+              spy.mockRestore()
+            }
+          }),
+        ),
+      { config: { lsp: true } },
     ),
   )
 
-  it.live("would spawn builtin LSP for files inside instance", () =>
+  it.live("does not spawn builtin LSP for files inside instance when LSP is unset", () =>
     provideTmpdirInstance((dir) =>
       LSP.Service.use((lsp) =>
         Effect.gen(function* () {
@@ -44,7 +46,7 @@ describe("lsp.spawn", () => {
               line: 0,
               character: 0,
             })
-            expect(spy).toHaveBeenCalledTimes(1)
+            expect(spy).toHaveBeenCalledTimes(0)
           } finally {
             spy.mockRestore()
           }
@@ -52,4 +54,56 @@ describe("lsp.spawn", () => {
       ),
     ),
   )
+
+  it.live("would spawn builtin LSP for files inside instance when lsp is true", () =>
+    provideTmpdirInstance(
+      (dir) =>
+        LSP.Service.use((lsp) =>
+          Effect.gen(function* () {
+            const spy = spyOn(LSPServer.Typescript, "spawn").mockResolvedValue(undefined)
+
+            try {
+              yield* lsp.hover({
+                file: path.join(dir, "src", "inside.ts"),
+                line: 0,
+                character: 0,
+              })
+              expect(spy).toHaveBeenCalledTimes(1)
+            } finally {
+              spy.mockRestore()
+            }
+          }),
+        ),
+      { config: { lsp: true } },
+    ),
+  )
+
+  it.live("would spawn builtin LSP for files inside instance when config object is provided", () =>
+    provideTmpdirInstance(
+      (dir) =>
+        LSP.Service.use((lsp) =>
+          Effect.gen(function* () {
+            const spy = spyOn(LSPServer.Typescript, "spawn").mockResolvedValue(undefined)
+
+            try {
+              yield* lsp.hover({
+                file: path.join(dir, "src", "inside.ts"),
+                line: 0,
+                character: 0,
+              })
+              expect(spy).toHaveBeenCalledTimes(1)
+            } finally {
+              spy.mockRestore()
+            }
+          }),
+        ),
+      {
+        config: {
+          lsp: {
+            eslint: { disabled: true },
+          },
+        },
+      },
+    ),
+  )
 })

+ 34 - 2
packages/opencode/test/lsp/lifecycle.test.ts

@@ -46,17 +46,49 @@ describe("LSP service lifecycle", () => {
     ),
   )
 
-  it.live("hasClients() returns true for .ts files in instance", () =>
+  it.live("hasClients() returns false for .ts files in instance when LSP is unset", () =>
     provideTmpdirInstance((dir) =>
       LSP.Service.use((lsp) =>
         Effect.gen(function* () {
           const result = yield* lsp.hasClients(path.join(dir, "test.ts"))
-          expect(result).toBe(true)
+          expect(result).toBe(false)
         }),
       ),
     ),
   )
 
+  it.live("hasClients() returns true for .ts files in instance when lsp is true", () =>
+    provideTmpdirInstance(
+      (dir) =>
+        LSP.Service.use((lsp) =>
+          Effect.gen(function* () {
+            const result = yield* lsp.hasClients(path.join(dir, "test.ts"))
+            expect(result).toBe(true)
+          }),
+        ),
+      { config: { lsp: true } },
+    ),
+  )
+
+  it.live("hasClients() keeps built-in LSPs when config object is provided", () =>
+    provideTmpdirInstance(
+      (dir) =>
+        LSP.Service.use((lsp) =>
+          Effect.gen(function* () {
+            const result = yield* lsp.hasClients(path.join(dir, "test.ts"))
+            expect(result).toBe(true)
+          }),
+        ),
+      {
+        config: {
+          lsp: {
+            eslint: { disabled: true },
+          },
+        },
+      },
+    ),
+  )
+
   it.live("hasClients() returns false for files outside instance", () =>
     provideTmpdirInstance((dir) =>
       LSP.Service.use((lsp) =>

+ 2 - 2
packages/sdk/js/src/v2/gen/types.gen.ts

@@ -1589,7 +1589,7 @@ export type Config = {
         }
   }
   formatter?:
-    | false
+    | boolean
     | {
         [key: string]: {
           disabled?: boolean
@@ -1601,7 +1601,7 @@ export type Config = {
         }
       }
   lsp?:
-    | false
+    | boolean
     | {
         [key: string]:
           | {