Skip to content
This repository has been archived by the owner on Mar 15, 2024. It is now read-only.

Commit

Permalink
refactor: new format marker %O which try calling String
Browse files Browse the repository at this point in the history
This marker replaces the %e format which is exclusivelly for errors. The advantage of %O over %e is that it never fails. This is handy because formatting is often use for debugging so we avoid reporting error while reporting another error (harder to understand). Note that %O can trigger arbitrary code whereas %o is always safe to use.
  • Loading branch information
lachrist committed Feb 15, 2022
1 parent 244e266 commit d74baff
Show file tree
Hide file tree
Showing 22 changed files with 53 additions and 66 deletions.
2 changes: 1 addition & 1 deletion components/abomination/default/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export default (dependencies) => {
await unlinkAsync(link);
await symlinkAsync(`${path}.cjs`, link, "file");
})(),
"Something went wrong when resolving the missing file extension issue >> %e",
"Something went wrong when resolving the missing file extension issue >> %O",
);
}
}
Expand Down
2 changes: 1 addition & 1 deletion components/batch/default/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export default (dependencies) => {
resolve({ signal, status });
});
}),
"Child error %j >> %e",
"Child error %j >> %O",
description,
);
subprocess = null;
Expand Down
2 changes: 1 addition & 1 deletion components/configuration-accessor/default/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ export default (dependencies) => {
const { scenarios, scenario } = configuration;
const regexp = expectSuccess(
() => new _RegExp(scenario, "u"),
"Scenario configuration field is not a valid regexp: %j >> %e",
"Scenario configuration field is not a valid regexp: %j >> %O",
scenario,
);
return scenarios
Expand Down
2 changes: 1 addition & 1 deletion components/configuration-environment/default/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export default (dependencies) => {
const { APPMAP_CONFIGURATION: content } = env;
const configuration = expectSuccess(
() => parseJSON(content),
"failed to parse 'APPMAP_CONFIGURATION' environment variable >> %e",
"failed to parse 'APPMAP_CONFIGURATION' environment variable >> %O",
);
validateConfiguration(configuration);
return configuration;
Expand Down
4 changes: 2 additions & 2 deletions components/configuration-process/node/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,15 @@ export default (dependencies) => {
const { code } = error;
expect(
code === "ENOENT",
"Cannot read configuration file at %j >> %e",
"Cannot read configuration file at %j >> %O",
url,
error,
);
return {};
}
return expectSuccess(
() => parse(content),
"Failed to parse configuration file at %j >> %e",
"Failed to parse configuration file at %j >> %O",
url,
);
};
Expand Down
12 changes: 6 additions & 6 deletions components/expect-inner/default/index.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ try {
}

assertEqual(
expectSuccess(() => 123, "%e"),
expectSuccess(() => 123, "%O"),
123,
);

Expand All @@ -42,19 +42,19 @@ try {
() => {
throw new Error("foo");
},
"%s %e",
"%s %O",
"bar",
);
assertFail();
} catch ({ message }) {
assertEqual(message, "bar foo");
assertEqual(message, "bar Error: foo");
}

await expectSuccessAsync(Promise.resolve(123), "%e");
await expectSuccessAsync(Promise.resolve(123), "%O");

try {
await expectSuccessAsync(Promise.reject(new Error("foo")), "%s %e", "bar");
await expectSuccessAsync(Promise.reject(new Error("foo")), "%s %O", "bar");
assertFail();
} catch ({ message }) {
assertEqual(message, "bar foo");
assertEqual(message, "bar Error: foo");
}
4 changes: 2 additions & 2 deletions components/expect/debug/index.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const { expect, expectSuccess, expectSuccessAsync } = Expect(
);
assertEqual(expect(true, "%s", "foo"), undefined);
assertEqual(
expectSuccess(() => 123, "%e"),
expectSuccess(() => 123, "%O"),
123,
);
await expectSuccessAsync(Promise.resolve(123), "%e");
await expectSuccessAsync(Promise.resolve(123), "%O");
2 changes: 1 addition & 1 deletion components/hook-query/node/require.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export default (dependencies) => {
try {
return createRequire(url)(name);
} catch (error) {
logWarning("Could not load %j from %j >> %e", name, directory, error);
logWarning("Could not load %j from %j >> %O", name, directory, error);
return null;
}
},
Expand Down
2 changes: 1 addition & 1 deletion components/hook-response/node/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ export default (dependencies) => {
recorder,
regexp: expectSuccess(
() => new _RegExp(intercept_track_port, "u"),
"Failed to compile the 'intercept-track-port' configuration field %j as regexp >> %e",
"Failed to compile the 'intercept-track-port' configuration field %j as regexp >> %O",
intercept_track_port,
),
};
Expand Down
2 changes: 1 addition & 1 deletion components/instrumentation/default/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export default (dependencies) => {
ecmaVersion: configuration.language.version,
locations: true,
}),
"failed to parse file %j >> %e",
"failed to parse file %j >> %O",
url,
),
{
Expand Down
2 changes: 1 addition & 1 deletion components/receptor-file/default/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export default (dependencies) => {
const { code } = error;
expect(
code === "ENOENT",
"cannot read directory status %j >> %e",
"cannot read directory status %j >> %O",
directory,
error,
);
Expand Down
2 changes: 1 addition & 1 deletion components/recorder-api/default/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export default (dependencies) => {
url = _String(url);
expectSuccess(
() => new _URL(url),
"the second argument of appmap.recordScript should be a valid url, got: %j >> %e",
"the second argument of appmap.recordScript should be a valid url, got: %j >> %O",
url,
);
return { type, url, content };
Expand Down
4 changes: 2 additions & 2 deletions components/repository/node/git.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export default (dependencies) => {
const error = coalesce(result, "error", null);
expect(
error === null,
`command %j on cwd %j threw an error >> %e`,
`command %j on cwd %j threw an error >> %O`,
command,
url,
error || { message: "dummy" },
Expand Down Expand Up @@ -74,7 +74,7 @@ export default (dependencies) => {
if (
!expectSuccess(
() => readdirSync(new _URL(url)),
"could not read repository directory %j >> %e",
"could not read repository directory %j >> %O",
url,
).includes(".git")
) {
Expand Down
8 changes: 4 additions & 4 deletions components/repository/node/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export default (dependencies) => {
const { code } = { code: null, ...error };
expect(
code === "ENOENT",
"failed to attempt reading package.json >> %e",
"failed to attempt reading package.json >> %O",
error,
);
return false;
Expand All @@ -35,14 +35,14 @@ export default (dependencies) => {
"utf8",
);
} catch (error) {
logWarning("Cannot read package.json file at %j >> %e", url, error);
logWarning("Cannot read package.json file at %j >> %O", url, error);
return null;
}
let json;
try {
json = parseJSON(content);
} catch (error) {
logWarning("Failed to parse package.json file at %j >> %e", url, error);
logWarning("Failed to parse package.json file at %j >> %O", url, error);
return null;
}
const { name, version, homepage } = {
Expand Down Expand Up @@ -75,7 +75,7 @@ export default (dependencies) => {
let url = pathToFileURL(
expectSuccess(
() => resolve(segments.join("/")),
"could not resolve %j from %j >> %e",
"could not resolve %j from %j >> %O",
segments,
home,
),
Expand Down
2 changes: 1 addition & 1 deletion components/serialization/default/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ export default (dependencies) => {
try {
serial.print = value.toString();
} catch (error) {
logWarning("%o.toString() failed with %e", value, error);
logWarning("%o.toString() failure >> %O", value, error);
serial.print = apply(toString, value, noargs);
}
} else {
Expand Down
2 changes: 1 addition & 1 deletion components/service/default/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export default (dependencies) => {
sockets.add(socket);
/* c8 ignore start */
socket.on("error", (error) => {
logWarning("Socket error >> %e", error);
logWarning("Socket error >> %O", error);
});
/* c8 ignore stop */
socket.on("close", () => {
Expand Down
2 changes: 1 addition & 1 deletion components/source/default/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export default (dependencies) => {
url,
expectSuccess(
() => parseJSON(content),
"Invalid JSON format for source map at %j >> %e",
"Invalid JSON format for source map at %j >> %O",
url,
),
);
Expand Down
2 changes: 1 addition & 1 deletion components/specifier/default/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export default (dependencies) => {
} else {
const regexp = expectSuccess(
() => new _RegExp(source, flags),
"failed to compile regexp source = %j flags = %j >> %e",
"failed to compile regexp source = %j flags = %j >> %O",
source,
flags,
);
Expand Down
4 changes: 2 additions & 2 deletions components/trace/appmap/classmap/estree/parse.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,12 @@ export default (dependencies) => {
attachComment: true,
});
} catch (error) {
logError("Unrecoverable parsing error at file %j >> %e", path, error);
logError("Unrecoverable parsing error at file %j >> %O", path, error);
return { type: "Program", body: [], sourceType: "script" };
}
const { errors, program: node } = result;
for (const error of errors) {
logWarning("Recoverable parsing error at file %j >> %e", path, error);
logWarning("Recoverable parsing error at file %j >> %O", path, error);
}
return node;
},
Expand Down
28 changes: 8 additions & 20 deletions components/util/default/format.mjs
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import { assert } from "./assert.mjs";
import { print } from "./print.mjs";

const { getOwnPropertyDescriptor } = Reflect;
const _undefined = undefined;
const _String = String;
const { stringify } = JSON;

export const format = (template, values) => {
Expand All @@ -21,27 +20,16 @@ export const format = (template, values) => {
assert(typeof value === "string", "expected a string for format");
return value;
}
if (marker === "e") {
assert(
typeof value === "object" && value !== null,
"expected an object",
);
const descriptor = getOwnPropertyDescriptor(value, "message");
assert(descriptor !== _undefined, "missing 'message' property");
assert(
getOwnPropertyDescriptor(descriptor, "value") !== _undefined,
"'message' property is an accessor",
);
const { value: message } = descriptor;
assert(
typeof message === "string",
"expected 'message' property value to be a string",
);
return message;
}
if (marker === "j") {
return stringify(value);
}
if (marker === "O") {
try {
return _String(value);
} catch {
return print(value);
}
}
if (marker === "o") {
return print(value);
}
Expand Down
27 changes: 13 additions & 14 deletions components/util/default/format.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,17 @@ assertEqual(format("%j", [[123]]), "[123]");

assertEqual(format("%o", [() => {}]), "[object Function]");

// %e //

assertEqual(format("%e", [new Error("foo")]), "foo");

assertThrow(() => format("%e", [null]), /^AssertionError: expected an object/u);

assertThrow(
() => format("%e", [{}]),
/^AssertionError: missing 'message' property/u,
);

assertThrow(
() => format("%e", [{ message: 123 }]),
/^AssertionError: expected 'message' property value to be a string/u,
// %O //

assertEqual(format("%O", [{ toString: () => "foo" }]), "foo");

assertEqual(
format("%O", [
{
toString: () => {
throw new Error("BOUM");
},
},
]),
"[object Object]",
);
2 changes: 1 addition & 1 deletion components/validate-appmap/on/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export default (dependencies) => {
validateAppmap: (data) => {
expectSuccess(
() => validateAppmap(data, { version: "1.6.0" }),
"failed to validate appmap\n%j\n%e",
"failed to validate appmap\n%j\n%O",
data,
);
},
Expand Down

0 comments on commit d74baff

Please sign in to comment.