feat: Add language to appmap.yml
#152
Conversation
test/codeBlock/appmap.yml
Outdated
| - path: . | ||
| exclude: | ||
| - node_modules |
There was a problem hiding this comment.
Because appmap-node always puts this exclude entry in the generated config file, you could consider testing for the presence of “node_modules” in the config file string before performing the language fix up. Probably not needed but would add a bit of protection.
There was a problem hiding this comment.
It's only setting language if it's currently missing. In other words, if it's set to something like ruby, nothing will happen. Do you think we need to go beyond that?
By default, this is set to `javascript`. Any existing configurations will be migrated automatically.
dustinbyrne
force-pushed
the
feat/add-language-appmap-yml
branch
from
facc4e1 to
a9f37b3
Compare
test/libraryCalls/appmap.yml
Outdated
| exclude: | ||
| - warn | ||
| - assert # exclude because this is captured only in windows No newline at end of file | ||
| - assert # exclude because this is captured only in windows | ||
| language: javascript |
There was a problem hiding this comment.
These changes automatically migrated during yarn jest
|
|
||
| function readConfigFile(document: Document): ConfigFile { | ||
| const config = document.toJSON() as Record<string, unknown>; |
There was a problem hiding this comment.
Please don't do a forced coercion like that. Instead, coerce to unknown and let asserts below carve the actual type.
There was a problem hiding this comment.
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.
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.
OK but I mean, so what? If the file is somehow mangled or corrupted, it's still going to error.
| 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.
You probably only need type Document here.
| YAML.stringify({ | ||
| name: "test-package", | ||
| appmap_dir: "appmap", | ||
| packages: [{ path: "." }], |
There was a problem hiding this comment.
Can we add a test of migration with user comments, to check they're preserved?
There was a problem hiding this comment.
I would say skip this. Let's get this out the door.
|
🎉 This PR is included in version 2.23.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
By default, this is set to
javascript. Any existing configurations will be migrated automatically.