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

refactor: glob by default for commands #492

Merged
merged 1 commit into from
Jul 27, 2016
Merged

Conversation

nfischer
Copy link
Member

This PR accomplishes:

  • globStart defaults to 1
  • commands have argument globbing by default
  • to opt out of globbing, use allowGlobbing: 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 uses common.expand(), this will have a slight conflict with #490 (this is just because I renamed expand to objectAssign 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.

canReceivePipe: false,
cmdOptions: false,
globStart: 1,
wrapOutput: false,
Copy link
Contributor

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.

@nfischer nfischer force-pushed the refactor-glob-start branch from 4ac06a4 to f8edfad Compare July 27, 2016 18:49
@nfischer
Copy link
Member Author

This now also makes wrapOutput default to true, and refactors accordingly.

@nfischer
Copy link
Member Author

Gonna give this an LGTM since it just covers the refactors discussed. @ariporad chime in if there are any other issues.

@nfischer
Copy link
Member Author

The only difference, I suppose, is that this removes the exception thrown if pipeOnly and canReceivePipe conflict (since it didn't work as well when specifying default options). We can provide documentation instead saying that pipeOnly overrides canReceivePipe (which is probably also a reasonable behavior).

@nfischer nfischer merged commit d411176 into master Jul 27, 2016
@nfischer nfischer deleted the refactor-glob-start branch July 27, 2016 23:43
@gyandeeps
Copy link
Contributor

Upgrade from 0.7.2 to 0.7.3 broke .test() function since it has allowGlobbing to true now.

  • Was this intentional?
    • If yes, Why was this breaking change introduced with a minor/patch version?

@nfischer nfischer self-assigned this Oct 17, 2016
@nfischer
Copy link
Member Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants