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 --cflags style option parameters, unit test, fixes #174 #176

Merged
merged 2 commits into from
Nov 21, 2013

Conversation

tonylukasavage
Copy link
Contributor

Based on the discussion already in #174, I removed the extra validation in required options so that they could handle cflag style option parameters. I also modified the normalize() function so that no additional normalization is performed on arguments following a require option. This accounts for a case like this:

COMMAND --cflags "-DDEBUG" 

without adjusting the normalization code, that would be normalized to:

COMMAND --cflags -D -D -E -B -U -G

but with the changes in this pull request it is handled appropriately.

All unit tests, included the one I added for this particular scenario, pass.

@tj
Copy link
Owner

tj commented Oct 23, 2013

hmmm not sure how I feel about it since -DEBUG on its own wouldn't be legal (if not following another flag etc), might lead to some weird confusion but probably not a huge deal

@tonylukasavage
Copy link
Contributor Author

@visionmedia -DEBUG isn't meant to be legal on its own, nor should it be. It's supposed to be literal input to the require option. I would imagine in almost all cases the CLI would wrap the arguments to the required option like this:

COMMAND --cflags "-DDEBUG -DSOMETHINGELSE" --other -yxz

It would think the potential confusion would be minimal, as anyone encountering the syntax would be using a project in which that syntax is known and defined. If I'm using a CLI tool for a project I know accepts cflag style options, I don't think seeing --cflags -DDEBUG would be off-putting. That said, I was personally stylistically encourage the use of quotes around the value.

@tonylukasavage
Copy link
Contributor Author

@visionmedia I had another idea, but I didn't want to take the time to implement it without your input. I thought rather than as a rule having all required options take the next argument as a literal, perhaps there could be a config option (like flag, description, coerce, default) for options to indicate that they should take the next argument as a literal, only in that case would they avoid the additional optionMissingArgument() check, avoiding confusion in all but the specifically designated cases.

I didn't want to screw with your current option() format though without seeing how you thought that might be done, since its style for me is one of the high points of this module. Essentially, you'd be able to specify in the option() call that this particular option takes the next argument as a literal no matter what it is. Personally, I think that required options are enough to indicate that, but if you feel there would be confusion, perhaps this would be a positive compromise.

Thoughts?

@tonylukasavage
Copy link
Contributor Author

Any thoughts here @visionmedia? I'd love to use commander in our current project, but this is a blocker for our purposes. Again, I'd be happy to implement the alternative I mentioned as well.

@tj
Copy link
Owner

tj commented Nov 21, 2013

ill merge for now it should be ok

tj added a commit that referenced this pull request Nov 21, 2013
Support for `--cflags` style option parameters, unit test, fixes #174
@tj tj merged commit 1317a02 into tj:master Nov 21, 2013
@tonylukasavage tonylukasavage deleted the 174-1 branch November 21, 2013 18:20
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