-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add language
to appmap.yml
#152
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`Config migrate does not update an existing config if the language field is already set 1`] = ` | ||
"name: test-package | ||
language: ruby | ||
appmap_dir: appmap | ||
packages: | ||
- path: . | ||
" | ||
`; | ||
|
||
exports[`Config migrate sets the language field in existing configs 1`] = ` | ||
"name: test-package | ||
appmap_dir: appmap | ||
packages: | ||
- path: . | ||
language: javascript | ||
" | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,16 @@ | ||
import assert from "node:assert"; | ||
import { readFileSync } from "node:fs"; | ||
import { readFileSync, writeFileSync } from "node:fs"; | ||
import { basename, join, resolve } from "node:path"; | ||
import { cwd } from "node:process"; | ||
|
||
import { PackageJson } from "type-fest"; | ||
import YAML from "yaml"; | ||
import YAML, { Document } from "yaml"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You probably only need |
||
|
||
import { warn } from "./message"; | ||
import { info, warn } from "./message"; | ||
import PackageMatcher, { Package, parsePackages } from "./PackageMatcher"; | ||
import locateFileUp from "./util/findFileUp"; | ||
import lazyOpt from "./util/lazyOpt"; | ||
import tryOr from "./util/tryOr"; | ||
import { isNativeError } from "node:util/types"; | ||
|
||
const responseBodyMaxLengthDefault = 10000; | ||
const kResponseBodyMaxLengthEnvar = "APPMAP_RESPONSE_BODY_MAX_LENGTH"; | ||
|
@@ -24,6 +23,10 @@ export class Config { | |
public readonly default: boolean; | ||
public readonly packages: PackageMatcher; | ||
public readonly responseBodyMaxLength: number; | ||
public readonly language: string; | ||
|
||
private readonly document?: Document; | ||
private migrationPending = false; | ||
|
||
constructor(pwd = cwd()) { | ||
const configDir = locateFileUp("appmap.yml", process.env.APPMAP_ROOT ?? pwd); | ||
|
@@ -35,13 +38,17 @@ export class Config { | |
const root = (this.root = process.env.APPMAP_ROOT ?? configDir ?? packageDir() ?? pwd); | ||
|
||
this.configPath = join(root, "appmap.yml"); | ||
const config = readConfigFile(this.configPath); | ||
this.default = !config; | ||
this.document = loadConfigDocument(this.configPath); | ||
const config = this.document ? readConfigFile(this.document) : undefined; | ||
this.default = !this.document; | ||
|
||
this.relativeAppmapDir = config?.appmap_dir ?? "tmp/appmap"; | ||
|
||
this.appName = config?.name ?? targetPackage()?.name ?? basename(root); | ||
|
||
this.language = config?.language ?? "javascript"; | ||
this.migrationPending ||= !!config && config.language === undefined; | ||
|
||
this.packages = new PackageMatcher( | ||
root, | ||
config?.packages ?? [ | ||
|
@@ -115,20 +122,38 @@ export class Config { | |
toJSON(): ConfigFile { | ||
return { | ||
name: this.appName, | ||
language: this.language, | ||
appmap_dir: this.relativeAppmapDir, | ||
packages: this.packages, | ||
}; | ||
} | ||
|
||
migrate() { | ||
if (!this.migrationPending) return; | ||
|
||
if (this.document) { | ||
this.document.set("language", this.language); | ||
} | ||
|
||
info("appmap.yml requires migration, changes will be automatically applied."); | ||
writeFileSync( | ||
this.configPath, | ||
YAML.stringify(this.document ?? this, { keepSourceTokens: true }), | ||
); | ||
} | ||
} | ||
|
||
interface ConfigFile { | ||
appmap_dir?: string; | ||
name?: string; | ||
packages?: Package[]; | ||
response_body_max_length?: number; | ||
language?: string; | ||
} | ||
|
||
function readConfigFile(path: string | undefined): ConfigFile | undefined { | ||
// Maintaining the YAML document is important to preserve existing comments and formatting | ||
// in the original file. | ||
function loadConfigDocument(path: string | undefined): Document | undefined { | ||
if (!path) return; | ||
|
||
let fileContent: string; | ||
|
@@ -139,22 +164,24 @@ function readConfigFile(path: string | undefined): ConfigFile | undefined { | |
throw exn; | ||
} | ||
|
||
let config; | ||
try { | ||
config = YAML.parse(fileContent) as unknown; | ||
} catch (exn) { | ||
assert(isNativeError(exn)); | ||
const document = YAML.parseDocument(fileContent, { keepSourceTokens: true }); | ||
if (document.errors.length > 0) { | ||
const errorMessage = document.errors.map((e) => `${e.name}: ${e.message}`).join("\n"); | ||
throw new Error( | ||
`Error parsing config file at ${path}: ${exn.message}\nYou can remove the file to use the default configuration.`, | ||
`Failed to parse config file at ${path}\n${errorMessage}\nYou can remove the file to use the default configuration.`, | ||
); | ||
} | ||
if (!config) return; | ||
return document; | ||
} | ||
|
||
function readConfigFile(document: Document): ConfigFile { | ||
const config = document.toJSON() as Record<string, unknown>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't do a forced coercion like that. Instead, coerce to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be fair, TypeScript doesn't provide great options for this situation. When loading data from a file, there is not much alternative to type coercion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not true. What I suggested is literally how to do it and it works really well: const input: unknown = JSON.parse(inputText);
assert(input && typeof input === "object");
// typescript knows input is object here
if ("key" in input && typeof input.key === "string")
process(input.key /* TypeScript knows it's a string here! */); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK but I mean, so what? If the file is somehow mangled or corrupted, it's still going to error. |
||
const result: ConfigFile = {}; | ||
assert(typeof config === "object"); | ||
if ("name" in config) result.name = String(config.name); | ||
if ("appmap_dir" in config) result.appmap_dir = String(config.appmap_dir); | ||
if ("packages" in config) result.packages = parsePackages(config.packages); | ||
if ("language" in config) result.language = String(config.language); | ||
if ("response_body_max_length" in config) { | ||
const value = parseInt(String(config.response_body_max_length)); | ||
result.response_body_max_length = value >= 0 ? value : undefined; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
name: codeBlock | ||
appmap_dir: tmp/appmap | ||
appmap_dir: tmp/appmap | ||
language: javascript |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
name: http-client-appmap-node-test | ||
appmap_dir: tmp/appmap | ||
response_body_max_length: 50 | ||
language: javascript |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
name: http-server-appmap-node-test | ||
appmap_dir: tmp/appmap | ||
language: javascript |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
name: jest-appmap-node-test | ||
appmap_dir: tmp/appmap | ||
language: javascript |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
name: mocha-appmap-node-test | ||
appmap_dir: tmp/appmap | ||
language: javascript |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
name: mysql-appmap-node-test | ||
appmap_dir: tmp/appmap | ||
language: javascript |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,3 +5,4 @@ packages: | |
exclude: | ||
- node_modules | ||
- .yarn | ||
language: javascript |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
name: prisma-appmap-node-test | ||
appmap_dir: tmp/appmap | ||
appmap_dir: tmp/appmap | ||
language: javascript |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,8 @@ | ||
name: simple | ||
appmap_dir: tmp/appmap | ||
packages: | ||
- path: . | ||
exclude: | ||
- skipped | ||
- A.skippedMethod | ||
- path: . | ||
exclude: | ||
- skipped | ||
- A.skippedMethod | ||
language: javascript |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
name: sqlite-appmap-node-test | ||
appmap_dir: tmp/appmap | ||
language: javascript |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,3 +5,4 @@ packages: | |
exclude: | ||
- node_modules | ||
- .yarn | ||
language: javascript |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
name: appmap-node | ||
appmap_dir: tmp/appmap | ||
language: javascript |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
name: vitest-appmap-node-test | ||
appmap_dir: tmp/appmap | ||
language: javascript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test of migration with user comments, to check they're preserved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say skip this. Let's get this out the door.