feat: exit command
#4
Conversation
There was a problem hiding this comment.
Thanks for this @pocketken! Yeah I'd rather land all this with tests so we don't break anything in the future. That said, if this PR was broken up into three separate PRs (one for the cd bug fix, one for test, and one for exit), then we can merge a few things faster than others and I can help get some tests in (just let me know where you'd like help).
src/commands/exit.ts
Outdated
|
|
||
| export function exitCommand(args: string[]): ExitExecuteResult { | ||
| const code = parseInt(args[0], 10); | ||
| if (isNaN(code)) throw new Error("invalid result code"); |
There was a problem hiding this comment.
Instead of throwing, we should probably log this message to stderr (, stderr: ShellPipeWriter) then return the same exit code that the exit command does in this scenario (I think 1, but my wsl is broken at the moment so I can't easily check).
There was a problem hiding this comment.
Looks like 2:
#!/bin/bash
exit aksjdksaj
$ bash ./t.sh
./t.sh: line 2: exit: aksjdksaj: numeric argument required
$ printf '%d\n' $?
2
src/common.ts
Outdated
| @@ -57,3 +59,9 @@ export function filterEmptyRecordValues<TValue>(record: Record<string, TValue | | |||
| } | |||
| return result; | |||
| } | |||
|
|
|||
| export function resolvePath(cwd: string, arg: string) { | |||
| return path.isAbsolute(arg) | |||
There was a problem hiding this comment.
Oh, I just assumed path.join handled this by default. Thanks!
There was a problem hiding this comment.
No problem. I also hoped that cd would take care of ~ and I was tempted to also include that in here -- but I was not sure if you wanted to add the dependency on std/node/os or not, so I handled it on my side. Are you open to the idea for the cd PR?
There was a problem hiding this comment.
That would be good, but I think std/node/os requires an --unstable flag to work. We could just copy this function into this library: https://deno.land/std@0.149.0/node/os.ts?code#L161
src/commands/exit.ts
Outdated
| import { ExitExecuteResult } from "../result.ts"; | ||
|
|
||
| export function exitCommand(args: string[]): ExitExecuteResult { | ||
| const code = parseInt(args[0], 10); |
There was a problem hiding this comment.
Also, there's some more logic we'll have to do with this: https://github.com/denoland/deno_task_shell/blob/18324b91faa83f94d84ac0c97766a08811d9c4ef/src/shell/commands/exit.rs#L26
There was a problem hiding this comment.
OK cool, will work from that for the tests as well. My Rust is (I won't say it.. its just too easy) rough, so, I'll let you know if I have any questions.
Sure, no problem. Give me a few and I'll at least get the |
|
@dsherret |
I just wanted to say this is a great little project, thanks for contributing it! It's coming in quite handy for a little project I am working on for migrating from
go-task.I came across a bug while trying to use
cd-- it assumes that all paths are relative to the CWD, which obviously leads to some weird behaviour when you do feed it absolute paths. I just added a quick check to leave absolute paths alone if provided, and that seems to do the trick.I also needed
exitand various incantations oftestfor my use case, so I added them as well. I know its a new project and that you're planning on adding support for more of the commands withindeno_task_shellat some point, so maybe these aren't exactly what you're looking for.. but they fit the bill for my use case, so I thought I might as well PR them back as well just in case anyone else needs them in the meantime.I apologize for being lazy and not adding tests; I just kind of made sure it worked for what I needed. I am ok with adding some tests as well if you'd like, but it may take me a couple of days.
Thanks again for the cool project!