feat: add `$.commandExists`, `$.dedent`, and `$.stripAnsi`

andrewbrey commented 1 year ago

Thank you very much for dax! I really love using it and it has been rock solid as a basis for a rewrite I've just completed of my dotfiles repository (https://github.com/andrewbrey/dotfiles). As you can see if you poke around that repo, it's a lot more than just dotfiles - it's more like a personal toolkit to take a computer from fresh OS install to set up exactly as I prefer.

In creating this repo, I found myself wishing that dax had just a few more helpers dangling off of $ because of how ubiquitous the $ import was in my modules and how commonly I encountered a few things like:

Check if a path is missing (the inverse of $.exists)
Check if a command is defined/undefined (based off of $.which)
Check if an environment variable is well defined
De-indent (dedent) template strings for printing formatted log messages

This PR adds some of the most useful of these to the $ "helpers". I think that each of these is very common in many kinds of scripts that deal with shell input/output, and having them readily available with a bit less boilerplate required would be quite helpful!

Cheers!

EDIT: note that the implementation of dedent chosen is the one referenced by the stage 2 TC39 proposal-string-dedent within the "playground" link, in specific, this one https://github.com/jridgewell/string-dedent . The string-dedent module is maintained by one of the two champions of the referenced TC39 proposal :+1:

andrewbrey commented 1 year ago

@dsherret Thanks for the notes and for taking a look!

(Also, just a note in the future and not a big deal, but it would be better to submit a PR per new API so that each feature can be merged individually)

Sure thing, if I have future PR's I'll be sure to scope them down to individual API changes per PR (didn't know if you would prefer more PRs to review or more changes in a single PR and opted to keep it all in one since all changes were to the same helpers object this time - happy to go the other way in the future!) :+1:

Negation Helpers

With respect to API additions for negation logic, the motivation for me is around ergonomics when using the async APIs, in particular in conditionals:

// to me, this feels easier to read and telegraphs its intent a bit clearer
if(await $.missing('somefile.txt')){}

// compared to this which requires more parens to work with the awaited expression
if(!(await $.exists('somefile.txt'))){}

I think this boils down mostly to an aesthetic argument and to a lesser extent an argument of "making intent easy to parse at a glance" when using async helpers.

I am happy to remove these "negation" helpers from the PR if this is not convincing to you, but figured I would toss it back over the fence for you to consider with that motivation elucidated a bit more.

ENV helpers

I've removed these two APIs from the PR since you stated you would prefer users just use Deno.env directly :+1:

Dedent

Any issue with just vendoring this function wholesale out of the NPM module if I can't find a deno userland module to supplant this one? There are a lot of edge cases to consider with the logic and that's why I pulled in a dependency in the first place rather than trying to write it myself (and also why I picked the one written by the TC39 champion). Either way, I'll get the npm: specifier dependency out of the PR :+1:

Strip Ansi

Great, I'll look into pulling this from the existing dependencies and I'll remove the npm: specifier dependency from the PR :+1:

andrewbrey commented 1 year ago

Update - I found a good deno.land/x module to replace string-dedent and swapped out strip-ansi with the analogous function from the wasmInstance. Let me know what you think about my comments related to "negation logic" :smile:

dsherret commented 1 year ago

Negation Helpers

It's a slippery slope to add them. Also, the parenthesis are not necessary. You can just write:

if (!await $.exists('somefile.txt')){
}

That said, specifically for file system apis, it's fine and faster to just use the synchronous APIs unless you're using the code in a web server or the file system might not be local.

andrewbrey commented 1 year ago

...how did I not know you don't need the () :exploding_head:

Ok - I'll remove the negated versions of these!

Also, looks like I need to fix some lint errors, so I'll push an update with that too :+1:

dsherret / dax

feat: add `$.commandExists`, `$.dedent`, and `$.stripAnsi` #71