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

Fix tags for --count argument #3195

Merged
merged 1 commit into from
Apr 28, 2023
Merged

Fix tags for --count argument #3195

merged 1 commit into from
Apr 28, 2023

Conversation

florelis
Copy link
Member

@florelis florelis commented Apr 27, 2023

This fixes the tags on the --count argument, which were causing an error during arg validation when it was used.

Closes #3194

Microsoft Reviewers: Open in CodeFlow

@florelis florelis requested a review from a team as a code owner April 27, 2023 23:00
@microsoft-github-policy-service microsoft-github-policy-service bot added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Apr 27, 2023
@@ -48,7 +48,7 @@ namespace AppInstaller::CLI
case Execution::Args::Type::Source:
return { type, "source"_liv, 's', ArgTypeCategory::Source };
case Execution::Args::Type::Count:
return { type, "count"_liv, 'n', ArgTypeCategory::MultiplePackages };
return { type, "count"_liv, 'n', ArgTypeCategory::PackageQuery | ArgTypeCategory::SinglePackageQuery };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how MultiplePackages is incorrect from an English semantics standpoint (I'm not sure how it differs from PackageQuery either though, which itself seems like an issue).

This feels like it is masking the actual issue since there is no way that count applies to SinglePackageQuery. I don't think we are interested in providing an "I'm feeling lucky" experience.

From looking more into the ArgTypeCategory design, what is needed is to make Args::Type::Query into Args::Type::QueryForSingle and Args::Type::QueryForMany (as example names). This will allow for different categories to match the different semantics for the commands.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I suppose we haven't done what I suggested for the name, etc. so maybe we don't want to do that for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was related to a previous feature to support installing multiple packages with 1 command invocation.

winget install TestApp1 TestApp2 is categorized as Multiple
winget install TestApp1 is categorized as SinglePackageQuery

The naming may seem a bit confusing with the concept of querying for single/multiple package

@florelis florelis merged commit f74a4e0 into microsoft:master Apr 28, 2023
@florelis florelis deleted the countarg branch April 28, 2023 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Count argument is throwing an error
3 participants