-
Notifications
You must be signed in to change notification settings - Fork 735
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
refactor: glob by default for commands #492
Conversation
canReceivePipe: false, | ||
cmdOptions: false, | ||
globStart: 1, | ||
wrapOutput: false, |
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 vote for this to default to true.
4ac06a4
to
f8edfad
Compare
This now also makes |
Gonna give this an LGTM since it just covers the refactors discussed. @ariporad chime in if there are any other issues. |
The only difference, I suppose, is that this removes the exception thrown if |
Upgrade from
|
@gyandeeps thanks for pointing this out. It looks like the change in behavior was a mistake. I'll review this more carefully and revert any changes to behavior. |
This PR accomplishes:
globStart
defaults to 1allowGlobbing: false
Commands have been refactored to take advantage of this new default value.
Also, this PR begins to store default values in a JS object for each
wrapOption
, which will probably make it easier to keep track of the defaults in the long run. Because this usescommon.expand()
, this will have a slight conflict with #490 (this is just because I renamedexpand
toobjectAssign
in that PR). I'll resolve the conflict after either PR is merged and update accordingly.This might be a good time to discuss if we want
wrapOutput
to be true by default, since it seems like it'll be the norm for a command to use that option.