#16: Adding documentation for most important devcmd API methods #27
Conversation
lieberlois
changed the title
| export interface ConsoleLike { | ||
| log(message?: any, ...optionalParams: any[]): void; | ||
| error(message?: any, ...optionalParams: any[]): void; | ||
| } | ||
|
|
||
| class SafeConsoleLike implements ConsoleLike { | ||
| constructor(private readonly consoleLike: ConsoleLike | undefined | null) {} | ||
| constructor(private readonly consoleLike: ConsoleLike | undefined | null) { } |
There was a problem hiding this comment.
hmmm, when I opened and saved this file on my machine, the space between the curlies here, and some other lines (e.g. trailing spaces) were removed.
please ensure that you have prettier and the prettier plugin for VS code installed, and that auto-format-on-save works for you (it should be enabled in the workspace settings), so that we can keep the styling consistent here
There was a problem hiding this comment.
update: this now also failed the lint-check in our GH pipeline 🙊
I've approved your changes to run in our pipeline, so I think further updates should automatically get checked there as well :)
There was a problem hiding this comment.
I only had Prettier installed in my WSL2 instance of VSCode, but not within the development container from #26 ^^
| /** | ||
| * Constructor | ||
| * | ||
| * @param {ConsoleLike} consoleLike A {@link ConsoleLike} for logging purposes |
There was a problem hiding this comment.
the type annotations in JSDoc are redundant, because we use TS and have the information there.
tbh i'm not sure how other consumers of JSDoc handle this in TS files, but VS Code doesn't even show these type annotations. I think we can just remove them -- they're just likely to become outdated if we ever change the code itself.
There was a problem hiding this comment.
Removing the type annotations
| @@ -29,9 +33,17 @@ class SafeConsoleLike implements ConsoleLike { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * A class abstracting the execution of tasks in separate child processes | |||
There was a problem hiding this comment.
I would even drop the "task" reference and phrase this slightly more generally. something like "Facilities to execute single or multiple external processes, with flexible forwarding of child process output"
There was a problem hiding this comment.
Thanks :) I adjusted the comment to your suggestion
| @@ -43,6 +55,20 @@ export class ProcessExecutor { | |||
| /** | |||
| * Executes a process and throws an exception if the exit code is non-zero. | |||
| * Outputs (stdout/stderr) of the process are sent to our stdout/stderr. | |||
| * | |||
| * @param {ProcessInfo} processInfo Information about the process that should be started | |||
There was a problem hiding this comment.
minor: in phrases like this, i find it slightly more readable to say "...the process to start" than "...that should be started" (furthermore, consistently using "execute" instead of "start" here would also be nice)
There was a problem hiding this comment.
Using start for comments.
Note: the logs say "Starting", should we change that to "Executing" aswell?
There was a problem hiding this comment.
I think in the logs it's fine (because these logs are user-facing, while the methods and docs here are dev-facing)
| @@ -51,6 +77,25 @@ export class ProcessExecutor { | |||
| /** | |||
| * Executes multiple processes in parallel and throws an exception if the exit code is non-zero. | |||
| * Outputs (stdout/stderr) of the processes are sent to our stdout/stderr. | |||
| * | |||
| * @param {{ [id: string]: ProcessInfo } | { [id: number]: ProcessInfo }} processMap A map linking process ids to {@link ProcessInfo} instances | |||
There was a problem hiding this comment.
good thing we talk about this!
the type and name of this parameter might be slightly intransparent, but aside from a name-based map, you can also just pass an array of ProcessInfo here (the int-indexed object type is compatible with arrays).
example (feel free to add this to process.test.ts):
test("succeeding processes as array works", async () => {
const execPipedParallel = new ProcessExecutor(nullConsole).execPipedParallel;
await execPipedParallel([
{ command: "node", args: ["--version"] },
{ command: withCmdOnWin("npm"), args: ["--version"] },
]);
});Therefore, a slightly more details description here might be worthwhile, e.g. something like "The {@link ProcessInfo}s to execute. Can be a ProcessInfo array or a map-object. In both cases, the indices are used to identify the child processes in log outputs."
There was a problem hiding this comment.
Thats interesting, i didnt know that!
I thought you would always have to provide ids yourself!
I added a descriptive comment that explains this (copied/inspired by your comment here ^^)
| return "ok" in r && r.ok; | ||
| } | ||
|
|
||
| /** | ||
| * Executes an awaitable function and wraps the result of the Promise |
There was a problem hiding this comment.
nit-pick: in JS/TS, every value is awaitable (it just immediately yields the value itself, e.g. await 2 works and evaluates to 2). maybe "a promise-returning function" is more apt ;)
There was a problem hiding this comment.
Damn it TypeScript, why cant you be easy on me one single time ;-)
| return "ok" in r && r.ok; | ||
| } | ||
|
|
||
| /** | ||
| * Executes an awaitable function and wraps the result of the Promise |
There was a problem hiding this comment.
I think we should refer to the unwrap function and their combined uses here, otherwise this function isn't very meaningful.
maybe something like "... and wraps the result for later use with {@link unwrap}"
and please also add a similar docstring to the unwrap function :)
This pullrequest adds JSDoc strings to the most important API parts of devcmd.
Example:
