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

Type stringOrEmpty not working as expected #1949

Open
scifipony opened this issue Apr 7, 2020 · 17 comments
Open

Type stringOrEmpty not working as expected #1949

scifipony opened this issue Apr 7, 2020 · 17 comments
Labels
bug Something isn't working good first issue Good for newcomers priority-low Legit issue but cosmetic or nice-to-have severity-low Bug that makes the usage of the Zowe less convenient but doesn't impact key use cases

Comments

@scifipony
Copy link

It appears that type stringOrEmpty when defining options is erroneously being validated the same as string. Here is what I found.

    public static EJES_OPTION_HELP_APP: ICommandOptionDefinition = {
            name: "helpApp",
            aliases: ["ha"],
            description: "Invoke application specific help.",
            type: "stringOrEmpty",
//            defaultValue: "usage",
            group: EjesProfile.GLOBAL_OPTIONS
        };

But when I run the Zowe, I get this:

PS C:\Users\Robert\projects\eclipse\zowe> zowe ejes query status --helpApp                                                                         
Syntax Error:
No value specified for option:
--helpApp

This option requires a value of type:
stringOrEmpty

Option Description:
Invoke application specific help.
:

Just in case the "option requires a value" means a null string, I've tried issuing that:


PS C:\Users\Robert\projects\eclipse\zowe> zowe ejes query status --helpApp ""                                                                     
Syntax Error:
No value specified for option:
--helpApp

This option requires a value of type:
stringOrEmpty

Option Description:
Invoke application specific help.
:

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.

 GLOBAL OPTIONS
 --------------

   --helpApp  | --ha (string)

      Invoke application specific help.

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.

PS C:\Users\Robert\projects\eclipse\zowe> zowe ejes query status --helpApp refine                                                                  

HELP FOR "Refining the First Row"
The number of rows populating a table depends on selection criteria.  The --table
and --report options operate only on the first row of a table.  To best use these
:

This appears to be a bug. Please fix it.

@scifipony
Copy link
Author

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.

        /**
         * Prevent empty string options, regardless of if they are
         * required or not  e.g.   --bright-zosmf-profile (without a value)
         */
        if (!util_1.isNullOrUndefined(this.mCommandDefinition.options)) {
            for (const option of this.mCommandDefinition.options) {
                if (!util_1.isNullOrUndefined(commandArguments[option.name]) &&
                    (option.type !== "stringOrEmpty" && commandArguments[option.name] === "") ||
                    (option.type !== "boolean" && commandArguments[option.name] === true)) {
                    valid = false;
                    this.emptyValueError(responseObject, option.name);
                }
            }
        }

@ghost
Copy link

ghost commented Apr 7, 2020

Due to the way Imperative parses CLI args (using the yargs package), an empty string cannot be specified like this:
zowe ejes query status --helpApp

Instead an empty string must explicitly be passed:
zowe ejes query status --helpApp ""

The only type of CLI arg that can be used without providing an associated value is a boolean option.

@scifipony
Copy link
Author

scifipony commented Apr 7, 2020

I will apologize ahead of time for the tone of my reply, but I am truly flabbergasted.

Due to the way Imperative parses CLI args (using the yargs package), an empty string cannot be specified like this:
zowe ejes query status --helpApp

Really?

Please note that I wrote:

Just in case the "option requires a value" means a null string, I've tried issuing that:
PS C:\Users\Robert\projects\eclipse\zowe> zowe ejes query status --helpApp ""

@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.

@scifipony
Copy link
Author

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:

PS C:\Users\Robert\projects\eclipse\zowe> zowe ejes emulate batch --helpApp ""

Syntax Error:
No value specified for option:
--helpApp

This option requires a value of type:
stringOrEmpty (which may be "<space>")

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.

@ghost
Copy link

ghost commented Apr 7, 2020

@tjohnsonBCM That workaround does not work.

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:

extension: {
name: "extension",
aliases: ["e"],
description: strings.EXTENSION,
type: "stringOrEmpty"
},
/**
* The directory to download all members to
* @type {ICommandOptionDefinition}
*/
directory: {
name: "directory",
aliases: ["d"],
description: strings.DIRECTORY,
type: "string"
},

Results of running zowe zos-files download all-members HLQ.PUBLIC.JCL --extension "":

$ zowe files dl am HLQ.PUBLIC.JCL --extension ""
Data set downloaded successfully.
Destination: hlq/public/jcl

Results of running zowe zos-files download all-members HLQ.PUBLIC.JCL --directory "":

$ zowe files dl am HLQ.PUBLIC.JCL --directory ""

Syntax Error:
No value specified for option:
--directory

This option requires a value of type:
string

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?

May I suggest writing a parser in Typescript to replace it? Parsers are programming 101 level work and shouldn't take long.

There has been a lot of careful consideration given to how Imperative parses CLI arguments. We are aware of some flaws in the yargs package and have considered writing our own parser in the future (#224). Please keep in mind the CLI squad has limited capacity, but contributions are always welcome 😄

@scifipony
Copy link
Author

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:

In formal language theory, the empty string, or empty word is the unique string of length zero.

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.,:

stringOrEmpty requires an argument that must be a string, or a 
blank string that is a blank enclosed in double-quotes.  

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.

@dkelosky
Copy link
Contributor

dkelosky commented Apr 7, 2020

hey @scifipony - can you also test @tjohnsonBCM's example? He shared a command that accepts a blank with zero spaces:

$ zowe files dl am HLQ.PUBLIC.JCL --extension ""
Data set downloaded successfully.
Destination: hlq/public/jcl

Does your definition using stringOrEmpty match this one?

Thanks,

@scifipony
Copy link
Author

@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 (""):

  • In CMD.EXE, Zowe receives what it expects.
  • In Powershell, it fails with "This options requires a value of type: stringOrEmpty".

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.

C:\Users\Robert\Projects\Eclipse\Zowe>zowe zos-files dl ds "rblum1.clist(pr17384)" -e

Syntax Error:
No value specified for option:
--extension

This option requires a value of type:
stringOrEmpty

Option Description:
Save the local files with a specified file extension. For example, .txt. Or ""
for no extension. When no extension is specified, .txt is used as the default
file extension.

Example:

 - Download the data set "ibmuser.loadlib(main)" in binary mode to the local file
 "main.obj":

      $ zowe zos-files download data-set "ibmuser.loadlib(main)" -b -f main.obj

Use "zowe zos-files dl ds --help" to view command description, usage, and options.

@dkelosky
Copy link
Contributor

dkelosky commented Apr 8, 2020

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!

@dkelosky
Copy link
Contributor

dkelosky commented Apr 8, 2020

@tjohnsonBCM - is there anything we can do for the powershell issue?

@scifipony
Copy link
Author

@dkelosky

Can you open an issue for the array problem[...]?

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:

  • "string" : string input that does not allow "" as a valid input value
  • "stringOrEmpty" : string input that does allow a "" as a valid input value

@dkelosky
Copy link
Contributor

dkelosky commented Apr 8, 2020

Sorry I misinterpreted:

I suspect you are going to want to open a second issue report.

Is this second issue to capture testing improvements?

@ghost
Copy link

ghost commented Apr 8, 2020

@tjohnsonBCM - is there anything we can do for the powershell issue?

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 "":

  • `"`"
  • "''"
  • '""'
  • [String]::Empty

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.

@scifipony
Copy link
Author

@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.


/**
 * The type of value that should be specified for an option by the user.
 * "array": an array of space delimited strings
 * "boolean": a switch - the user specifies:  true "--option-name" or false: "--option-name false"
 * "count" : accepting only whole numbers as input value
 * "existingLocalFile": a file for which fs.existsSync returns true
 * "json": a parseable JSON string
 * "number" : accepting integers as input value
 * "string" : string input that does not allow "" as a valid input value
 * "stringOrEmpty" : string input that does allow a "" as a valid input value
 * ** Warning: Powershell users instead of "" must use `"`", "''", '""', or [String]::Empty
 */

@scifipony
Copy link
Author

@dkelosky re: Issues:

Is this second issue to capture testing improvements?

No

Can you open an issue for the array problem and for alternate wording proposal on the option help?

  1. I do not see a problem with receiving an empty array. It's a feature. A string of length 0 is valid data, as is an array of 0 elements.
  2. This issue has resolved to changing the comment on stringOrEmpty. Should I change the issue title to read "Documentation for stringOrEmpty is vague and requires more information"?

@dkelosky
Copy link
Contributor

dkelosky commented Apr 8, 2020

I like your original title :)

@gejohnston gejohnston added bug Something isn't working good first issue Good for newcomers priority-low Legit issue but cosmetic or nice-to-have severity-low Bug that makes the usage of the Zowe less convenient but doesn't impact key use cases labels Dec 13, 2022
@gejohnston
Copy link
Member

If our resolution is to add additional help text to the CommandOptionType of stringOrEmpty, then this would be a good first issue.

@awharn awharn transferred this issue from zowe/imperative Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers priority-low Legit issue but cosmetic or nice-to-have severity-low Bug that makes the usage of the Zowe less convenient but doesn't impact key use cases
Projects
Status: Low Priority
Development

No branches or pull requests

3 participants