search

heroku / legacy-cli

Heroku CLI
https://cli.heroku.com
MIT License
1.36k stars 381 forks source link

Parsing problem with CLI plugin #2030

Closed GaryFNC closed 7 years ago

GaryFNC commented 7 years ago

We have built a CLI plugin which has a flag with the following properties name: 'name', char: 'n', hasValue: true, required: true,

When the user specifies command -n xxx or command --name xxx, everything works fine

But, when the user specifies -nxxx, the run function in the command receives xxx for context.flags.name. Apparently, the parser doesnt know to reject -nxxx by requiring a space after the character shorthand

jdx commented 7 years ago

the space isn't required, is there a reason you think it should be?

GaryFNC commented 7 years ago

Sorry if I wasnt clear - I do understand that sometimes folks use the -abc form for multiple short chars, but from working with other CLIs over the years, that can be a dangerous pattern, IMHO.
In particular IF the --xxx starts with the same letter as the short char and takes an argument, it can be a troubling issue of easy mistakes.. I'd suggest that IF a flag/option requires a value, then it should always require being followed by a space. For example having -nab foo is going to be hard to understand even if 'a' & 'b' are short chars for non value holding options. And really odd if 'a' or 'b' also require a value

jdx commented 7 years ago

I don't think I agree. It's useful to be able to do things like heroku apps:restart -aappname.

GaryFNC commented 7 years ago

There are examples out there - and yes, commands arent consistent. grep does allow -file to be "ile", but doesnt allow -dir to be an action "ir". git will complain if you try to do git branch -dfoobar and asks if you meant --, but will recognize git branch --del foobar without the full delete. And it doesnt accept git -deletefoobar.... There are cases where you might want to combine single character flags that Do Not require a value - agreed and common.
But if you have flags that take a value, this gets messy fast.... ex. foo --name xxx --path zzz and foo -n xxx -p zzz, but what does it mean to do foo -nxxxpzzz or foo -npxxxzzz. This gets ambiguous and confused very quickly.
So, if you really want to allow combined single chars, that is viable and is commonly used. But when you have flags that allow both a '-' form and a '--form", it really calls out for not being able to mix those with non value flags and calls out for having a separator space....
Yes, we can work around this by making sure that the single char form cant feed into any double -- form, but that starts running out of characters and the natural tendency for a user is to accept the first char as the abbreviated form (-n --name, -p --path)

jdx commented 7 years ago

It's not possible to support multiple short flags with values in the same argument like -nxxxpzzz. There is no way for the parser to delimit. That is fine though since it's just optional shorthand.

I don't follow your last point if you want to elaborate. However I am going to close this ticket as this is working as designed.