Skip to content

Commit 9d1fe2c

Browse files
marcorothmarcoroth
authored
Config: Prefer .herb.yml over soft project indicators (#1602)
The `findProjectRootSync` and `findConfigFile` would stop at the first directory containing any project indicator (like `README.md`), even if `.herb.yml` existed higher up in the directory tree. This caused `exclude` patterns to be silently ignored when files were passed from subdirectories that happened to contain a `README.md` or similar indicator. Now both methods remember the first indicator match as a fallback but keep walking up looking for `.herb.yml`. If found, it takes priority. Resolves #1595
1 parent 8674857 commit 9d1fe2c

6 files changed

Lines changed: 123 additions & 11 deletions

File tree

‎javascript/packages/config/src/config.ts‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -503,6 +503,8 @@ export class Config {
503503
currentPath = path.resolve(process.cwd())
504504
}
505505

506+
let firstIndicatorMatch: string | undefined
507+
506508
while (true) {
507509
const configPath = path.join(currentPath, this.configPath)
508510

@@ -514,20 +516,23 @@ export class Config {
514516
// Config not in this directory, continue
515517
}
516518

517-
for (const indicator of this.PROJECT_INDICATORS) {
518-
try {
519-
fsSync.accessSync(path.join(currentPath, indicator))
519+
if (!firstIndicatorMatch) {
520+
for (const indicator of this.PROJECT_INDICATORS) {
521+
try {
522+
fsSync.accessSync(path.join(currentPath, indicator))
520523

521-
return currentPath
522-
} catch {
523-
// Indicator not found, continue checking
524+
firstIndicatorMatch = currentPath
525+
break
526+
} catch {
527+
// Indicator not found, continue checking
528+
}
524529
}
525530
}
526531

527532
const parentPath = path.dirname(currentPath)
528533

529534
if (parentPath === currentPath) {
530-
return process.cwd()
535+
return firstIndicatorMatch || process.cwd()
531536
}
532537

533538
currentPath = parentPath
@@ -864,6 +869,8 @@ export class Config {
864869
currentPath = path.resolve(process.cwd())
865870
}
866871

872+
let firstIndicatorMatch: string | undefined
873+
867874
while (true) {
868875
const configPath = path.join(currentPath, this.configPath)
869876

@@ -875,16 +882,18 @@ export class Config {
875882
// Config not in this directory, continue
876883
}
877884

878-
const isProjectRoot = await this.isProjectRoot(currentPath)
885+
if (!firstIndicatorMatch) {
886+
const isProjectRoot = await this.isProjectRoot(currentPath)
879887

880-
if (isProjectRoot) {
881-
return { configPath: null, projectRoot: currentPath }
888+
if (isProjectRoot) {
889+
firstIndicatorMatch = currentPath
890+
}
882891
}
883892

884893
const parentPath = path.dirname(currentPath)
885894

886895
if (parentPath === currentPath) {
887-
return { configPath: null, projectRoot: process.cwd() }
896+
return { configPath: null, projectRoot: firstIndicatorMatch || process.cwd() }
888897
}
889898

890899
currentPath = parentPath

‎javascript/packages/config/test/find-project-root.test.ts‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
import { describe, test, expect, beforeAll, afterAll } from "vitest"
2+
import { mkdirSync, writeFileSync, rmSync } from "fs"
3+
import { join } from "path"
4+
import { Config } from "../src/config.js"
5+
6+
describe("findProjectRootSync", () => {
7+
const tempDir = join(process.cwd(), "tmp-test-find-project-root")
8+
9+
beforeAll(() => {
10+
mkdirSync(join(tempDir, "app", "frontend", "components"), { recursive: true })
11+
mkdirSync(join(tempDir, "app", "views", "layouts"), { recursive: true })
12+
13+
writeFileSync(join(tempDir, ".herb.yml"), "linter:\n exclude:\n - 'app/views/**/*.html.erb'\n")
14+
writeFileSync(join(tempDir, "app", "frontend", "README.md"), "# Frontend\n")
15+
writeFileSync(join(tempDir, "app", "frontend", "components", "button.html.erb"), "<button></button>\n")
16+
writeFileSync(join(tempDir, "app", "views", "layouts", "application.html.erb"), "<html></html>\n")
17+
})
18+
19+
afterAll(() => {
20+
rmSync(tempDir, { recursive: true, force: true })
21+
})
22+
23+
test("finds project root with .herb.yml over subdirectory with README.md", () => {
24+
const filePath = join(tempDir, "app", "frontend", "components", "button.html.erb")
25+
const projectRoot = Config.findProjectRootSync(filePath)
26+
27+
expect(projectRoot).toBe(tempDir)
28+
})
29+
30+
test("finds project root from deeply nested file path", () => {
31+
const filePath = join(tempDir, "app", "views", "layouts", "application.html.erb")
32+
const projectRoot = Config.findProjectRootSync(filePath)
33+
34+
expect(projectRoot).toBe(tempDir)
35+
})
36+
37+
test("prefers .herb.yml over soft project indicators like README.md", () => {
38+
const filePath = join(tempDir, "app", "frontend", "components", "button.html.erb")
39+
const projectRoot = Config.findProjectRootSync(filePath)
40+
41+
expect(projectRoot).not.toBe(join(tempDir, "app", "frontend"))
42+
expect(projectRoot).toBe(tempDir)
43+
})
44+
})

‎javascript/packages/linter/test/__snapshots__/cli.test.ts.snap‎

Lines changed: 6 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎javascript/packages/linter/test/cli.test.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,31 @@ describe("CLI Output Formatting", () => {
287287
try { unlinkSync(configPath) } catch {}
288288
}
289289
})
290+
291+
test("skips excluded file in subdirectory with README.md project indicator", () => {
292+
const { mkdirSync, rmSync } = require("fs")
293+
const subdir = "test/fixtures/subdir"
294+
295+
try {
296+
mkdirSync(subdir, { recursive: true })
297+
writeFileSync(`${subdir}/README.md`, "# Subdir\n")
298+
writeFileSync(`${subdir}/excluded.html.erb`, '<img>\n')
299+
300+
writeFileSync(configPath, dedent`
301+
linter:
302+
exclude:
303+
- "subdir/**/*.html.erb"
304+
`)
305+
306+
const { output, exitCode } = runLinter("subdir/excluded.html.erb")
307+
308+
expect(output).toMatchSnapshot()
309+
expect(exitCode).toBe(0)
310+
} finally {
311+
try { unlinkSync(configPath) } catch {}
312+
try { rmSync(subdir, { recursive: true, force: true }) } catch {}
313+
}
314+
})
290315
})
291316

292317
describe("Multiple File Arguments", () => {

‎javascript/packages/linter/test/snapshot-serializer.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import { resolve } from "path"
2+
import type { SnapshotSerializer } from "vitest"
3+
4+
const projectRoot = resolve(__dirname, "..")
5+
const repoRoot = resolve(__dirname, "../../../..")
6+
7+
const serializer: SnapshotSerializer = {
8+
serialize(value: string, config, indentation, depth, refs, printer) {
9+
const normalized = value
10+
.replaceAll(repoRoot, "/test/herb")
11+
.replaceAll(projectRoot, "/test/herb/javascript/packages/linter")
12+
13+
return printer(normalized, config, indentation, depth, refs)
14+
},
15+
16+
test(value: unknown) {
17+
return typeof value === "string" && value.includes(projectRoot)
18+
},
19+
}
20+
21+
export default serializer

‎javascript/packages/linter/vitest.config.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
import { defineConfig } from "vitest/config"
2+
3+
export default defineConfig({
4+
test: {
5+
snapshotSerializers: ["./test/snapshot-serializer.ts"],
6+
},
7+
})

0 commit comments

Comments
 (0)