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

Refactor arg validation #2862

Merged
merged 9 commits into from
Jan 31, 2023
Merged

Refactor arg validation #2862

merged 9 commits into from
Jan 31, 2023

Conversation

florelis
Copy link
Member

@florelis florelis commented Jan 17, 2023

This change adds a set of argument "categories" that tell us what an argument is used for and makes it easier to reason around which arguments can be used at the same time. For example, a manifest arg cannot be used at the same time as an arg related to sources.

The main change is adding a function to identify the categories of arguments used in an invocation, and a function to use those categories for validation. Then, I used these new functions to replace the arg validation for commands that had similar rules.

This is what I wanted to do for #1788, and should make arg validation for #2861 easier to get right.

Microsoft Reviewers: Open in CodeFlow

@florelis florelis requested a review from a team as a code owner January 17, 2023 21:07
src/AppInstallerCLICore/Argument.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Argument.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Argument.h Show resolved Hide resolved
src/AppInstallerCLICore/Argument.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Argument.cpp Outdated Show resolved Hide resolved
@yao-msft
Copy link
Contributor

I was thinking to tag each arg with ArgTypeCategory so it's more clear and whenever we add new arg, we are more likely to think about it.

@florelis
Copy link
Member Author

I saw the comments about exclusive sets and using the categories in the context cloning, but I missed the part where it said it wasn't needed in this PR :D
So maybe I spent more in this than I should have but anyways...

I added a new type for the couple of fields in an argument that I'm using here: the name (for error messages), the categories and a new bitmask for exclusive sets. Since I added the name, I also put the alternate name and alias in here.
Then I added a ForType() function for this class, like the one that exists for Argument, but I added a test to enforce that all types are listed in this function. Using this function we can get all the information we needed to drive checking incompatible categories, exclusive sets, and we are now able to list the arg name in error messages for any arg.
The more complete Argument type now wraps this new smaller type to provide the name/alias, but we can still use a different help string or setting or policy on each command that uses the argument. Maybe it should have extended instead of wrapping, and maybe some other fields like the argument type (positional/flag) should have also been moved to the new smaller type.
My change adds the limitation that if two commands use the same arg type, it has to have the same name, but I don't think that will be an issue.

@Trenly
Copy link
Contributor

Trenly commented Jan 21, 2023

I saw the comments about exclusive sets and using the categories in the context cloning, but I missed the part where it said it wasn't needed in this PR :D So maybe I spent more in this than I should have but anyways...

I added a new type for the couple of fields in an argument that I'm using here: the name (for error messages), the categories and a new bitmask for exclusive sets. Since I added the name, I also put the alternate name and alias in here. Then I added a ForType() function for this class, like the one that exists for Argument, but I added a test to enforce that all types are listed in this function. Using this function we can get all the information we needed to drive checking incompatible categories, exclusive sets, and we are now able to list the arg name in error messages for any arg. The more complete Argument type now wraps this new smaller type to provide the name/alias, but we can still use a different help string or setting or policy on each command that uses the argument. Maybe it should have extended instead of wrapping, and maybe some other fields like the argument type (positional/flag) should have also been moved to the new smaller type. My change adds the limitation that if two commands use the same arg type, it has to have the same name, but I don't think that will be an issue.

Wow, that's way more than I would have ever expected! A cursory glance looks amazing, but I'll look closer once I've got some time

src/AppInstallerCLICore/Argument.cpp Outdated Show resolved Hide resolved
@florelis florelis merged commit 5d0e02b into microsoft:master Jan 31, 2023
@florelis florelis deleted the validateArgs branch January 31, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants