Skip to content

Commit 6c72352

Browse files
[🔥AUDIT🔥] Make the error messages more helpful (#1796)
🖍 _This is an audit!_ 🖍 ## Summary: This updates the thrown error messages to be more helpful by including relevant information, such as the the starting path or the marker being looked for. Issue: Closes #1794 ## Test plan: `pnpm test` `pnpm lint` `pnpm typecheck` Author: somewhatabstract Auditors: copilot-pull-request-reviewer[bot], somewhatabstract Required Reviewers: Approved By: Checks: ✅ 8 checks were successful, ⏭️ 1 check has been skipped Pull Request URL: #1796
1 parent 66de34b commit 6c72352

File tree

5 files changed

+36
-12
lines changed

5 files changed

+36
-12
lines changed

.changeset/soft-planes-lead.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"ancesdir": major
3+
---
4+
5+
Now throws more detailed errors

.github/workflows/nodejs.yml

+3-2
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,9 @@ name: Node CI
22

33
on:
44
pull_request:
5-
branches:
6-
- main
5+
# ready_for_review is useful for when a PR is converted from "draft" to "not
6+
# draft".
7+
types: [edited, opened, synchronize, ready_for_review, reopened]
78

89
push:
910
branches:

jest.config.js

+2
Original file line numberDiff line numberDiff line change
@@ -20,4 +20,6 @@ module.exports = {
2020
},
2121

2222
setupFilesAfterEnv: ["jest-extended/all"],
23+
24+
prettierPath: null,
2325
};

src/__tests__/index.test.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ describe("ancesdir", () => {
1717

1818
// Assert
1919
expect(underTest).toThrowErrorMatchingInlineSnapshot(
20-
`"From path must be absolute"`,
20+
`"The starting path must be absolute, but "./path" is relative"`,
2121
);
2222
});
2323

@@ -44,7 +44,7 @@ describe("ancesdir", () => {
4444

4545
// Assert
4646
expect(underTest).toThrowErrorMatchingInlineSnapshot(
47-
`"No such marker found from given starting location"`,
47+
`"Could not find marker, "markerwewontfind", from given starting location "/Absolute/Path""`,
4848
);
4949
});
5050

@@ -59,7 +59,7 @@ describe("ancesdir", () => {
5959

6060
// Assert
6161
expect(underTest).toThrowErrorMatchingInlineSnapshot(
62-
`"No such marker found from given starting location"`,
62+
`"Could not find marker, "markerwewontfind", from given starting location "/Absolute/Path""`,
6363
);
6464
});
6565

src/index.ts

+23-7
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,21 @@ import * as fs from "fs";
33

44
import cache from "./cache";
55

6-
const Errors = {
7-
MARKER_NOT_FOUND: "No such marker found from given starting location",
8-
NOT_ABSOLUTE_PATH: "From path must be absolute",
9-
} as const;
6+
const throwStartingPathNotAbsoluteMessage = (from: string): never => {
7+
throw new Error(
8+
`The starting path must be absolute, but "${from}" is relative`,
9+
);
10+
};
11+
12+
const throwMarkerNotFoundMessage = (from: string, marker: string): never => {
13+
throw new Error(
14+
`Could not find marker, "${marker}", from given starting location "${from}"`,
15+
);
16+
};
1017

1118
function ancesdirImpl(from: string, marker: string): string {
1219
if (!path.isAbsolute(from)) {
13-
throw new Error(Errors.NOT_ABSOLUTE_PATH);
20+
throwStartingPathNotAbsoluteMessage(from);
1421
}
1522

1623
const keys = [];
@@ -31,6 +38,15 @@ function ancesdirImpl(from: string, marker: string): string {
3138

3239
/**
3340
* If we have already looked this up, we can just grab it from our map!
41+
*
42+
* NOTE: If the marker was created after we cached the result, we will
43+
* still claim it cannot be found. Equally, if the marker was deleted
44+
* after we cached the result, we will still claim it was found.
45+
* The assumption is that a use of ancesdir is short-lived and such
46+
* scenarios aren't likely to occur. We could mitigate this by adding
47+
* the ability to clear the cache, or by making the cache invalidate
48+
* after a certain time period. Something to consider for a future
49+
* update, though no one has asked for it yet.
3450
*/
3551
const cachedResult = cache.get(key);
3652
if (cachedResult != null) {
@@ -39,7 +55,7 @@ function ancesdirImpl(from: string, marker: string): string {
3955
} else if (cache.has(key)) {
4056
// A null in the cache means we tried this lookup and never found
4157
// the marker.
42-
throw new Error(Errors.MARKER_NOT_FOUND);
58+
throwMarkerNotFoundMessage(from, marker);
4359
}
4460

4561
/**
@@ -54,7 +70,7 @@ function ancesdirImpl(from: string, marker: string): string {
5470
for (const key of keys) {
5571
cache.set(key, null);
5672
}
57-
throw new Error(Errors.MARKER_NOT_FOUND);
73+
throwMarkerNotFoundMessage(from, marker);
5874
}
5975

6076
/**

0 commit comments

Comments
 (0)