-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Require selection argument on install/show/search/uninstall #2125
Conversation
…rt of selection criteria
{ | ||
if (args.Contains(type)) | ||
{ | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to ensure the query is also not an empty string?
#1249
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be possible, but allowing an empty string does allow for an effective --all
being ""
. The current change prevents accidental "everything" searches, which I feel was the primary value. Preventing intentional searching for everything seems like it would be due to the current performance limitations rather than due to protecting users.
If the user types in winget search ""
, it could be an accident, but it is more likely that it is intentional than winget search
. I could see scripts constructing the command line resulting in more frequent empty strings as well, but if the script author wants to block that, they can.
Without any other reasons, I'm not inclined to block the empty strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those are fair points, and are things I hadn't considered. After thinking more, I do agree that simply requiring an argument solves the potential issues here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #1131
Change
For
install
,show
,search
, anduninstall
commands, require that one of the selection arguments be provided. These are (where appropriate per command):I intentionally did not provide a way to override this behavior (like an
--all
forsearch
), because I don't see a good use case for it. If there is one, please explain it below.Microsoft Reviewers: Open in CodeFlow