Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from
Jan 7, 2023
Merged

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

merged 17 commits into from
Jan 7, 2023

Conversation

andrewbrey
Copy link
Contributor

@andrewbrey andrewbrey commented Jan 7, 2023

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 👍

Copy link
Owner

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @andrewbrey! $.commandExists(Sync), $.dedent, and $.stripAnsi look good, but I'm not too sure about the others.

(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. For example, I could have merged $.commandExists right now if they were separate)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/deps.ts Outdated Show resolved Hide resolved
@andrewbrey
Copy link
Contributor Author

@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!) 👍

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 👍

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 👍

Strip Ansi

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

@andrewbrey
Copy link
Contributor Author

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" 😄

@dsherret
Copy link
Owner

dsherret commented Jan 7, 2023

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
Copy link
Contributor Author

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

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 👍

src/deps.ts Outdated Show resolved Hide resolved
Copy link
Owner

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@dsherret dsherret changed the title feat: add useful scripting helpers feat: add $.commandExists, $.dedent, and $.stripAnsi Jan 7, 2023
@dsherret dsherret merged commit d09b483 into dsherret:main Jan 7, 2023
@andrewbrey andrewbrey deleted the ab/feat/more-helpers branch January 7, 2023 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants