-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add stdin option to command chains #198
Conversation
There is still some work to do (docs and tests). I wanted to see what are your thought 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.
Looks good, I tested this locally and it's working great. Just left some feedback on minor tweaks. In addition to adding the field in chain_source.md
, could you do a search for /sh.*-c/
in the docs (I found two results) and change those examples to use this new field? Thanks!
a5c3323
to
cc61658
Compare
Sorry, had to do some force pushes because my editor reformatted md files 😅 |
I have no idea for any new tests. I think it is ready to merge. |
Resolved conversations. Not sure about the test name. |
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.
Looks good, you have conflicts though so I can't merge it. Could you rebase onto master
please?
Never mind, I forgot I can also make edits. The conflicts where just on the changelog so I resolved and squashed. |
Description
Describe the change. If there is an associated issue, please include the issue link (e.g. "Closes #xxx"). For UI changes, please also include screenshots.
Added
stdin
field on the !command variant. Stdin is now piped into command usingChildStdin
. It is still compatible with previous command variant.Closes #190
Known Risks
What issues could potentially go wrong with this change? Is it a breaking change? What have you done to mitigate any potential risks?
Calling command may be unsafe, but I don't think it carries any new threat.
QA
How did you test this?
Added one unit test. Probably need to add more.
Checklist
CONTRIBUTING.md
already?CHANGELOG.md
?