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

Support for multi-word command line arguments #88

Merged
merged 2 commits into from
Sep 2, 2017
Merged

Support for multi-word command line arguments #88

merged 2 commits into from
Sep 2, 2017

Conversation

rgroothuijsen
Copy link
Contributor

This PR addresses #75 by allowing quote-mark separated command line arguments. It changes the splitting of argument strings to a 2-step process: it first splits the string apart by quotes (any part matching "*"), and then splits parts of the string up further if they are not part of a quote.

@rgroothuijsen
Copy link
Contributor Author

Hmm, npm test passes but nsp throws an error. Not sure what to make of that.

Copy link
Member

@SBoudrias SBoudrias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sending the PR. I think that's a pretty good feature to add!

@@ -506,6 +506,19 @@ class Environment extends EventEmitter {

return require.resolve(untildify(moduleId));
}

splitArgsFromString(argsString) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be on the prototype. Can you move it as a standalone function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where would be a good place to put it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The easiest would be to just move it at the top of the file. We can move it elsewhere if we have a need later on.

@SBoudrias SBoudrias merged commit 73637a6 into yeoman:master Sep 2, 2017
@SBoudrias
Copy link
Member

Awesome, thanks!!

@rgroothuijsen rgroothuijsen deleted the issue75 branch September 2, 2017 18:01
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