-
Notifications
You must be signed in to change notification settings - Fork 87
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
Type stringOrEmpty not working as expected #1949
Comments
My best guess as to what is wrong centers on a conditional logic error in this code in SyntaxValidator.js. Since I don't know exactly what all the bits here are doing, I'm not willing to bet on whether I am right or not.
|
Due to the way Imperative parses CLI args (using the Instead an empty string must explicitly be passed: The only type of CLI arg that can be used without providing an associated value is a boolean option. |
I will apologize ahead of time for the tone of my reply, but I am truly flabbergasted.
Really? Please note that I wrote:
@tjohnsonBCM That workaround does not work. If it did work, I would use it. Frankly. Think about it. What enterprise-level command line interface would work this way? What non-enterprise-level CLI would work this way? Command line programs have existed on "workstation platforms" since the early 1970's when Unix was developed with C. This and all subsequent platforms allowed developers to write what we now call options that optionally took an argument. That's because the developer parsed the argv or equivalent in their own code. I can see that stringOrEmpty was intended to fix a design flaw in the Zowe imperative. That some developer chose a "yargs" package to fast-track development of the command parser is not an excuse for a deficiency. May I suggest writing a parser in Typescript to replace it? Parsers are programming 101 level work and shouldn't take long. |
Addendum: Your first reply uses a null string, which does not include a space between the quotes, thus it did not work when I tried it. Including a space between the quotes does work(!), but it's still a very poor non-standard workaround. And, if it is intended as a workaround, I would suggest that the error read something like this to prevent confusion:
BTW, I still consider this a design flaw as detailed in my previous reply. My documentation will have to read that to specify a no-argument flag, you have to specify quote space quote. It is counter-intuitive. Zowe deserves a better parser. |
Apologies for missing this in your original comment. Thank you for clearly documenting the issue. I believe this may be an issue with the EJES plugin, as I've attempted to reproduce the issue in Imperative and was unsuccessful. Details below. The command "zowe zos-files download all-members" supports the following options: zowe-cli/packages/zosfiles/src/cli/download/Download.options.ts Lines 62 to 78 in ab02cfe
Results of running
Results of running
As can be seen in the command output above, passing a blank string for a CLI option of type "stringOrEmpty" is supported in Zowe CLI. I don't have access to the source code of the EJES plugin to check, but perhaps there is some validation code in the EJES plugin's command handler which is making it reject empty strings as valid input?
There has been a lot of careful consideration given to how Imperative parses CLI arguments. We are aware of some flaws in the |
Thank you for replying. As I noted in my addendum (which you likely missed, crossed in the mail as it were), the option does work with a string with one blank in it, and does not work with an empty string as you wrote in your original reply. It is therefore working as designed. BTW, let me quote Wikipedia:
A string with a blank has a length of one. The terminology of using empty in stringOrEmpty is unfortunate. It led to my confusion, for certain. The more explicit choice of stringOrBlankString doesn't roll off the tongue, either. Since it is not currently practical to rewrite the parser, I strongly suggest that you at minimum change the comments in the code about stringOrEmpty to explicitly state how it functions, i.e.,:
The best reason to take my advice is that it comes from my sad tale of woe. I looked at code to learn about the option, and I suspect most on-boarding developers will do so also. |
hey @scifipony - can you also test @tjohnsonBCM's example? He shared a command that accepts a blank with zero spaces:
Does your definition using Thanks, |
@dkelosky Son-of-a gun! This is a Windows 10 Powershell issue. Yes, my definition matches your cited one. @tjohnsonBCM When I use an empty string, that is double-quote double-quote with no blank enclosed (""):
Both terminals accept " ", i.e., double-quote space double-quote. This is one of those issues where the dev-team needs to test against all terminals on all platforms, or fix the code per platform, or come up with a more compatible solution. (As an aside, I am investigating using a type of array instead of stringOrEmpty. It allows the user to omit the argument. Problem is that it shows up as (array) in the help and accepts multiple arguments. Hey, why can't a stringOrEmpty be a single element array?) I suspect you are going to want to open a second issue report. Sorry. Please don't shoot the messenger. BTW, I see that the zos-files programmers saw the same issue I did and documented how to specify no argument without omitting it. Am I strange in thinking that's kind of weird? Read how they framed it in their Option Description below.
|
Noted on the need for more testing, although there are many terminals and many platforms 😀. Can you open an issue for the array problem and for alternate wording proposal on the option help? Thank you for your persistence in testing and for following up! |
@tjohnsonBCM - is there anything we can do for the powershell issue? |
Wait... what? I consider an omitted argument on array to be a feature. I am guessing others do, also. This could be a breaking change. I suggest the second line below be modified as shown in imperative/packages/cmd/src/doc/option/ICommandOptionDefinition.ts:
|
Sorry I misinterpreted:
Is this second issue to capture testing improvements? |
This seems to be an unfortunate deviation in the behavior of PowerShell compared to other shells. In PowerShell v3-7 (not sure about earlier versions), an empty argument cannot be passed to an executable without escaping it. This issue has been discussed on StackOverflow, and several ways have been suggested to escape the characters
Needing to escape the characters is not an ideal solution, however I tested all of these methods and they worked for passing a "stringOrEmpty" argument to Zowe CLI. |
@tjohnsonBCM Great investigation, and it demonstrates how problematic it is to support the stringOrEmpty when what is needed is to allow any operand type to optionally accept an omitted argument. (I can't help but think that if a developer added an ICommandOptionDefinition.defaultValue, the imperative should also allow the user to enter an operand of any type and omit the argument.) That said, the dev-team needs to add an ugly "footnote" to stringOrEmpty so developers will know what to document for their users. BTW, I use Powershell, so that's how I discovered the issue. Hover over my avatar for an advisement.
|
@dkelosky re: Issues:
No
|
I like your original title :) |
If our resolution is to add additional help text to the CommandOptionType of stringOrEmpty, then this would be a good first issue. |
It appears that type stringOrEmpty when defining options is erroneously being validated the same as string. Here is what I found.
But when I run the Zowe, I get this:
Just in case the "option requires a value" means a null string, I've tried issuing that:
It appears to me that the stringOrEmpty code is being ignored. My intuition is bolstered by the text of the generated help topic when I run Zowe with --help. Note that the option help below states string not stringOrEmpty, which is shown as coded in my option definition at the top of this issue report.
I've tried coding a default value (I've commented it out in my code example). I've tried using a different option name, like --info. None of this works. If I do specify an argument, the option works like a champ.
This appears to be a bug. Please fix it.
The text was updated successfully, but these errors were encountered: