feat!: parseNi etc. now return ResolvedCommand object instead of string, migrate to tinyexec
#231
Conversation
|
commit: |
|
We should probably wait for tinylibs/tinyexec#29 to be resolved first, but besides that it looks good to me |
src/runner.ts
Outdated
| @@ -140,5 +148,12 @@ export async function run(fn: Runner, args: string[], options: DetectOptions = { | |||
| return | |||
| } | |||
|
|
|||
| await ezspawn(command, { stdio: 'inherit', cwd }) | |||
| const [xCommand, ...xArguments] = command.split(' ') | |||
There was a problem hiding this comment.
I don't think it's safe to use split, as you might have edge cases like run "foo bar", we would either need to complete change how we compose the commands (use array instead of string), or we need tinyexec to support parsing string commands
There was a problem hiding this comment.
This should be easy since we have the original arguments, I'll change it tmr.
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
antfu
changed the title
tinyexecparseN.. now returns ResolvedCommand object instead of string, migreate to tinyexec
|
Made some refactoring and upgraded |
src/utils.ts
Outdated
| const VOLTA_PREFIX = 'volta run' | ||
| const hasVoltaCommand = cmdExists('volta') | ||
| return hasVoltaCommand ? VOLTA_PREFIX : '' | ||
| export function isVoltaExists(): boolean { |
There was a problem hiding this comment.
doesVoltaExist might be a bit more grammatically correct name
There was a problem hiding this comment.
I don't think identifiers are valid English sentences, which grammar doesn't always apply (it can be read as an abbr of "is it true that volta exists in current directory?"). I tend to use is prefix to describe functions that return boolean.
There was a problem hiding this comment.
fair enough. might be able to make it isVoltaInstalled or isVoltaPresent or something like that if you wanted to keep the is prefix and make it a bit more grammatical
anyway, this PR looks good to me. this was the only thing I noticed
antfu
changed the title
parseN.. now returns ResolvedCommand object instead of string, migreate to tinyexecparseNi etc. now return ResolvedCommand object instead of string, migrate to tinyexec
Description
This PR
parseNi,parseNretc programmatic APIs now returnsResolvedCommandobject instead of plain string@jsdevtools/ez-spawnwithtinyexecusingxarguments (split command using space).🚨🚨 DO NOT RELEASE YET WE NEED TO DO SOME EXTERNAL TESTS 🚨🚨
Linked Issues
Additional context