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

Confusing help text for -g/--ignore-globs option #567

Open
agr opened this issue Jul 7, 2023 · 7 comments
Open

Confusing help text for -g/--ignore-globs option #567

agr opened this issue Jul 7, 2023 · 7 comments
Labels
bug external Issues that require an external change to resolve

Comments

@agr
Copy link

agr commented Jul 7, 2023

Describe the bug
Running devskim analyze --help produces this line:

-g, --ignore-globs (Default: /.git/ /bin/) Comma-separated Globs for files to skip analyzing

Notice, the text says "Comma-separated", but the default value is space-separated. So, which is it?

To Reproduce
Steps to reproduce the behavior:

  1. Run devskim analyze --help
  2. Scan the output for the line mentioned above.

Expected behavior
Displayed defaults/examples should be consistent with description text.

Screenshots
image

Versions(please complete the following information):

  • OS: Windows 10 22H2 / 19045.3086
  • Devskim Version 1.0.11+87ad45b866
@agr agr added the bug label Jul 7, 2023
@agr
Copy link
Author

agr commented Jul 7, 2023

Also, help text has whole bunch of extra empty lines, so the output unnecessarily doesn't fit into one screen and I have to scroll up to see it in its entirety.

@gfs
Copy link
Contributor

gfs commented Jul 7, 2023

@agr

Sorry for the confusion. The correct way to provide the arguments on the command line is comma separated. This gets automatically split on commas by the CommandLineParser dependency, giving the application itself an IEnumerable<string>, with the strings subsequently converted to Globs for pattern matching.

You can see here where the globs argument is defined:

[Option('g', "ignore-globs", HelpText = "Comma-separated Globs for files to skip analyzing", Separator = ',', Default = new[] { "**/.git/**", "**/bin/**" })]

I believe the help text displays this as space separated because the actual default value is an IEnumerable as if it had already been parsed.

We can investigate if it possible to change how this is represented to the user, as I agree it may be confusing that the spaces are not rendered as part of the default.

As for the empty lines, do you see something like this, with one line break between each argument or are you seeing multiple line breaks between each argument? We can also look into if its possible to remove those line breaks between argument specifications, as there are a large number of arguments so condensing it may be helpful.

image

@agr
Copy link
Author

agr commented Jul 7, 2023

Yes, that's exactly what I am talking about. When I am experimenting with options and mess up so devskim doesn't actually do anything, it produces an error, then follows it with the help screen which overflows the output and I have to scroll up to see what went wrong. Just try running devskim analyze as an example.
Alternative here would be not to print help on failure maybe?

@gfs
Copy link
Contributor

gfs commented Jul 8, 2023

It looks like removing the extra newlines is a supported configuration change, but it requires a bit of refactoring as we currently use the default parser (https://github.com/commandlineparser/commandline/wiki/HelpText-Configuration).

Its not clear to me that it would be easy to change the default values to express the comma separator, but we can investigate that as well.

@daalcant

@agr
Copy link
Author

agr commented Jul 10, 2023

None of it is a big deal, just an inconvenience, so I don't expect any priority for that. And should the missing comma in defaults be actually reported to CommandLineParser?

@gfs
Copy link
Contributor

gfs commented Jul 11, 2023

@agr Thanks for your understanding.

As for the missing commas, that does seem to me like an oversight in the default help generator from command line parser.

I didn't immediately see any configurable option to address that - unlike removing the extra line - if there isn't one it's likely something that will need to addressed there first.

@gfs
Copy link
Contributor

gfs commented Jul 27, 2023

It looks like the separator missing from the Default value is already a reported issue in the CommandLine repo: commandlineparser/commandline#490

I opened a PR to resolve it: commandlineparser/commandline#895

@gfs gfs added the external Issues that require an external change to resolve label Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug external Issues that require an external change to resolve
Projects
None yet
Development

No branches or pull requests

2 participants