Skip to content

refactor: glob by default for commands#492

Merged
nfischer merged 1 commit intomasterfrom
refactor-glob-start
Jul 27, 2016
Merged

refactor: glob by default for commands#492
nfischer merged 1 commit intomasterfrom
refactor-glob-start

Conversation

@nfischer
Copy link
Copy Markdown
Member

@nfischer nfischer commented Jul 27, 2016

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.

ariporad
ariporad reviewed Jul 27, 2016
View reviewed changes
src/common.js Outdated
canReceivePipe: false,
cmdOptions: false,
globStart: 1,
wrapOutput: false,
Copy link
Copy Markdown
Contributor

@ariporad ariporad Jul 27, 2016

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
Copy Markdown
Member Author

nfischer commented Jul 27, 2016

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

@nfischer
Copy link
Copy Markdown
Member Author

nfischer commented Jul 27, 2016

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

@nfischer
Copy link
Copy Markdown
Member Author

nfischer commented Jul 27, 2016

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
Copy Markdown
Contributor

gyandeeps commented Oct 17, 2016

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
Copy Markdown
Member Author

nfischer commented Oct 17, 2016

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

@nfischer nfischer mentioned this pull request Oct 17, 2016
Closed
@gyandeeps gyandeeps mentioned this pull request Oct 17, 2016
Merged
@snyk-bot snyk-bot mentioned this pull request Sep 5, 2019
Open
@snyk-bot snyk-bot mentioned this pull request Sep 19, 2019
Open
@snyk-bot snyk-bot mentioned this pull request Nov 29, 2019
Open
@snyk-bot snyk-bot mentioned this pull request Mar 8, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@ariporad ariporad

@nfischer nfischer

Labels

low priority refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nfischer @gyandeeps @ariporad