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

Verify command line flags (by default) #3371

Open
2 tasks done
sdarwin opened this issue Dec 18, 2023 · 10 comments
Open
2 tasks done

Verify command line flags (by default) #3371

sdarwin opened this issue Dec 18, 2023 · 10 comments
Labels

Comments

@sdarwin
Copy link

sdarwin commented Dec 18, 2023

Checklist

  • I have verified this is the correct repository for opening this issue.
  • I have verified no other issues exist related to my request.

Is Your Feature Request Related To A Problem? Please describe.

It's easy to mistype any command. When that happens, it's helpful to get an informational error.

Here is an example:

choco install visualstudio2022buildtools --fake-command-line-flag

Chocolatey doesn't indicate the option is invalid?

Or

choco install visualstudio2022buildtools --package-parametersss

Almost correct. The same problem. No warning message. No error message. Just a "success".

Describe The Solution. Why is it needed?

Sanitize command line flags. Return a clear error message "invalid option : X" . Exit with an error code.

It's better to return an error, than to allow the user to mistakenly believe they are getting certain options, but the options were misspelled.

(the proposal is that this should occur 'by default'.)

Additional Context

No response

Related Issues

No response

@gep13
Copy link
Member

gep13 commented Dec 18, 2023

@sdarwin sounds like you want to make use of the ignoreInvalidOptionsSwitches feature flag.

https://docs.chocolatey.org/en-us/configuration#ignoreinvalidoptionsswitches

This is enabled by default, since there are ways of using Chocolatey CLI where you specifically want to pass in options that Chocolatey CLI doesn't know about.

However, for your use case, if you run:

choco feature disable --name="ignoreInvalidOptionsSwitches"

You should get the result you want.

Let me know if this answers your question, and we can close out this issue.

@sdarwin
Copy link
Author

sdarwin commented Dec 18, 2023

Hi @gep13 ,

This was implemented in issue 586. Comments from there:

like the way PowerShell ignores switches it doesn't understand

Well, if I run a powershell script it will give an error:

script.ps1 -helpz

A parameter cannot be found that matches parameter name 'helpz'

This allows passing things that would be created in future versions and not having older versions break when attempting to use those switches.

Usually if a new switch gets added in a future version, it becomes available in future versions. Older versions don't have access to that particular switch. But that is normal.

What is the most frequent and common case of needing to pass unknown options to choco? If 97% of users are not applying unknown options flags, those users would be better off if the default setting was catching typos. Rather than allowing mistakes to go undetected.

It would sort of make sense if this was the method to pass options directly to the underlying installer. However, to do that, you use --install-arguments or --package-parameters.

@gep13
Copy link
Member

gep13 commented Dec 19, 2023

@sdarwin said...
What is the most frequent and common case of needing to pass unknown options to choco?

One example would be the choco new command, where you can pass additional arguments that are passed in as templated variables. This is done in conjunction with the templating functionality that exists in Chocolatey CLI.

@sdarwin said...
If 97% of users are not applying unknown options flags, those users would be better off if the default setting was catching typos.

Due to how Chocolatey CLI works, i.e. no phone home, or telemetry, we simply don't know what the usage is.

If I have understood you correctly, what you are requesting is no longer that Chocolatey CLI detects that invalid options are passed in (since this functionality is already provided via the ignoreInvalidOptionsSwitches feature), but rather you are requesting that the default value of this feature be changed. If that is indeed the case, can I request that you go back and update the issue title and description to reflect that request.

@sdarwin
Copy link
Author

sdarwin commented Dec 19, 2023

If "new" must be able to take arbitrary switches, then treat "new" differently. Allow that command to have unknown options.

We can imagine, that most users are running "choco install", and most users don't know about the existence of "ignoreInvalidOptionsSwitches".

Default settings should be aimed at the largest set of users, the typical user.

There's a reason in Linux if you type ls --test that it says

ls: unrecognized option '--test'
Try 'ls --help' for more information.

Error code 2.

The problem with allowing unknown options is more than typos, it's also "misunderstandings". You construct a long, somewhat complicated installation command, believe it should be working, and there are no errors or warnings. Yet behind-the-scenes, it failed to do what you expected.

While I believe the command should output an error message and exit with an error code, like ls, there is compromise position. Display a warning message. Continue with the current behavior of attempting to run the command.


(intentionally left blank. add spaces, colors, or some way to call attention to the message)
 
WARNING. choco does not recognize the option '--test'. That option will not be processed. Attempting to proceed anyway.

Perhaps implement the warning-only mode first and leave that in place for a couple years. Then eventually enable errors. A phased approach.

If there is a case to be made for certain subcommands such as new those could be exceptions.

@sdarwin sdarwin changed the title Verify command line flags Verify command line flags (by default) Dec 19, 2023
Copy link

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?
This issue will be closed in 14 days if it continues to be inactive.

@github-actions github-actions bot added the Pending Closure This issue has been marked as having no response or is stale and will soon be closed. label Jan 19, 2024
@sdarwin
Copy link
Author

sdarwin commented Jan 19, 2024

This issue will be closed in 14 days if it continues to be inactive.

The "default" behavior of a command line tool should be to verify the provided --option-flags. If 10% or fewer subcommands really do need to accept any input, it could be triggered by that particular subcommand itself.

So let's say -z needs to be able to accept a bunch of unknown unvalidated command-line input. The presence of -z unlocks that behavior. Otherwise validate inputs.

@pauby
Copy link
Member

pauby commented Jan 19, 2024

@sdarwin @gep13 asked:

If I have understood you correctly, what you are requesting is no longer that Chocolatey CLI detects that invalid options are passed in (since this functionality is already provided via the ignoreInvalidOptionsSwitches feature), but rather you are requesting that the default value of this feature be changed. If that is indeed the case, can I request that you go back and update the issue title and description to reflect that request.

(emphasis mine). That hasn't been done, and that is why the issue was due to be closed.

To set expectations, the default behaviour of this option is already established behaviour, and changing it would cause unexpected issues for some users. We would appreciate any feedback on how we highlight that this option being available, to more people.

If I've misunderstood what you are asking, please let me know.

@sdarwin
Copy link
Author

sdarwin commented Jan 19, 2024

That hasn't been done, and that is why the issue was due to be closed.

I had already update the title, from "Verify command line flags" to -> "Verify command line flags (by default)".

Now I have added text into the first post: "(the proposal is that this should occur 'by default'.)

Implementing this feature request is more then toggling ignoreInvalidOptionsSwitches because gep13 mentioned choco new as an example of a command that might require unknown switches. Are there other examples of commands where unknown switches must be used? If so, those few cases would need to "ignore invalid options switches".

If there are currently any seldom-used, mysterious, but still necessary, switches that aren't documented, they should be added into the list of "valid command-line switches" in some way or another.

But generally I suspect that 95% of users are running "choco install", and 95% of users don't know about ignoreInvalidOptionsSwitches, and those same users will never know about ignoreInvalidOptionsSwitches.

Still the "average use case" would benefit from getting at least a warning when there's a problem with the command-line input.

I had suggested a compromise. Instead of having choco crash, display a clear warning message. Leave the warning message in place for a couple years. Before eventually continuing along the path of generating an error.

@pauby
Copy link
Member

pauby commented Jan 19, 2024

As I mentioned above, the behaviour of Chocolatey CLI is already established, whether it's believed to right or wrong. So it's not something we'd be changing.

Outside of making that change, I'm unclear what else needs to do be done here.

@sdarwin
Copy link
Author

sdarwin commented Jan 19, 2024

In that case...

How about a slightly different alternative then.

Instead of modifying ignoreInvalidOptionsSwitches, add a new setting, something like "enable warnings".

"enableWarningsInvalidOptionsSwitches". True by default.

It will "show warnings when invalid options switches are found". at least in my opinion, since the warnings are sort of serious, they should be offset by surrounding spaces. Try to draw attention to them.

@github-actions github-actions bot removed the Pending Closure This issue has been marked as having no response or is stale and will soon be closed. label Jan 20, 2024
@github-actions github-actions bot added the Pending Closure This issue has been marked as having no response or is stale and will soon be closed. label Feb 20, 2024
@pauby pauby removed 0 - Waiting on User Pending Closure This issue has been marked as having no response or is stale and will soon be closed. labels Feb 20, 2024
@chocolatey chocolatey deleted a comment from github-actions bot Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants