-
Notifications
You must be signed in to change notification settings - Fork 11
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
WIP: Added support for inline scripting #15
Conversation
} | ||
|
||
// I'm not sure if you have a loop preference? This is the method I have become accustom to in terms of readability and performance. | ||
envKeys.forEach((envKey: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The forEach
looks good for me
// This is a little ugly but I like explicit variable declaration for readability. | ||
const envValue: string = env[envKey]; | ||
// String are immutable so it doesn't matter if we directly work on the cmd string. | ||
cmd = cmd?.replace(`\${${envKey}}`, envValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about changing the format from ${ENV_NAME}
to $ENV_NAME
?
I prefer the second as it's the way POSIX commands utilize environment variables, it's the same way you would write in a bash terminal for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am completely fine with changing that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, in POSIX shells ${ENV_NAME}
== $ENV_NAME
.
While $ENV_NAME
is commonly used, ${ENV_NAME}
is less ambiguous when embedded within other text (eg, "${ENV_NAME}S" vs "$ENV_NAMES"). Given the possible ambiguities, you'll want to at least allow the ${ENV_NAME}
format.
In my PR I implemented basic env support in my code so I would have to just remove that and wait on your env support to be released. As of now I don't think there is anything but if you have any suggestions I'm open to them |
I think it's okay, once I finish the |
Description
Adding inline/Adhoc script support would allow devs to included much needs functionality seeing as no package manager is included, I believe this would enhance user experience at very little cost while allowing users to write short automation scripts.
I discovered this package while reading this article dev. I liked the package however it was not quite working how I needed it, nor how the OP was discussing.
Types of changes
Related links
https://dev.to/vonheikemen/a-simple-way-to-replace-npm-scripts-in-deno-4j0g
Changelog
Added