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

if args is falsy, assume a default collection. #209

Merged
merged 1 commit into from
Apr 25, 2014
Merged

if args is falsy, assume a default collection. #209

merged 1 commit into from
Apr 25, 2014

Conversation

v0lkan
Copy link
Contributor

@v0lkan v0lkan commented Apr 11, 2014

This is kind of an edge case that I coincide if I try to break things hard enough
while using commander at https://github.com/v0lkan/JFDI/

From tracing the code I tend to come to a conclusion that
when the command's option is not required, and the option is not optional, and we follow the

program
.command(..)
.description(..)
.action(function(){
});

pattern, then after some event chaining, args happens to be undefined which breaks the flow.

Providing a default value if args do not exist, will make the function less error-prone nonetheless; and it won't cause any regression in the existing behavior.

This is kind of an edge case.
When the command's option is not required, and the option is not optional, and we follow the

```
 *      program
 *        .command(..)
 *        .description(..)
 *        .action(function(){
 *        });
```

pattern, then after some event chaining args happens to be `undefined` which breaks the flow.

Providing a default value if args do not exist, will make the function less error-prone nonetheless; and it won't cause any regression in the existing behavior.
@v0lkan
Copy link
Contributor Author

v0lkan commented Apr 25, 2014

@visionmedia is there a reason that this does not get merged.

It's a one-liner, and it passes the CI tests.

If there's something to be done on my part, I'll be more than happy to do it.

And if there's a certain workflow that I'm not following, please do let me know.

Cheers.

tj added a commit that referenced this pull request Apr 25, 2014
if args is falsy, assume a default collection.
@tj tj merged commit fa2ce5c into tj:master Apr 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants