-
Notifications
You must be signed in to change notification settings - Fork 6
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
pass args to scripts #49
Conversation
- test wether pn properly prints and outputs additional args passed to echo
Any status on this? |
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 about the arguments that contain whitespaces? Don't we need to escape them before passing the command to sh -c
?
Related crates: https://docs.rs/shell-escape/0.1.5/shell_escape/
This crate is better: https://docs.rs/shell-quote/0.3.0/shell_quote/sh/fn.escape_into.html
Not sure what that would look like. care to give an example? How does pnpm do it? |
pnpm for sure handles whitespace. But for how pnpm did it, you must ask @zkochan. If you ask me, I have already answered: Just use the crate I suggested, I don't really care how exactly pnpm did it. |
Actually, I think this crate is better: https://docs.rs/shell-quote/0.3.0/shell_quote/sh/fn.escape_into.html You do have to change the type of |
i tried it out a few seconds ago but clap handles it. |
If you used my list-args.js, the issue would have been clearer. Also, please use the |
@KSXGitHub not sure if i did anything wrong but it doesnt seem to work and based on the ci, shell quote is failing to build on windows. |
This was an oversight on my part. I was not aware that shell-quote is not cross-platform. Let's try os_display. Also, changing the type of |
- remove shell-quote - add os-display
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.
feat: improve the code
merged |
CLRF strikes again. |
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.
Can you merge this? https://github.com/Yakiyo/pn/pull/2
feat: use `Cow` to reduce allocation
done |
Pass any additional arguments passed after script name in command invocation to the script.
Resolves #48