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 flags and configs for subcommands #64

Merged
merged 9 commits into from
Aug 18, 2024
Merged

Conversation

garg3133
Copy link
Member

@garg3133 garg3133 commented Aug 17, 2024

Naming conventions used in the PR:

  • command > subcommand > flag > configs
  • The android command has many subcommands, each representing an action that the user wants to perform.
  • Each subcommand can have several flags, which represent the area where the subcommand should act. Ex. install --app means that the "install" action should be performed for "app".
  • Corresponding to each flag, multiple configs can be passed directly through the CLI command. For ex. for install --app, there can be a --path (APK path) and a --deviceId (id of target device) config.
  • There can also be subcommands with no flags, but some configs that can be passed to the CLI command directly.

After this PR, we'll only use "options" to represent the options passed through the CLI. For other specific purposes, we'll use either "flag" or "configs".

Previous iteration of this PR: #51

Comment on lines +186 to +200
// set main config in `options` if config aliases are passed.
const aliasToMainConfig: {[key: string]: string} = {};
allowedConfigs.forEach(config => {
config.alias.forEach(alias => {
aliasToMainConfig[alias] = config.name;
});
});

configsPassed.forEach((configName) => {
if (aliasToMainConfig[configName]) {
// `configName` is an alias
const mainConfig = aliasToMainConfig[configName];
options[mainConfig] = options[configName];
}
});
Copy link
Member Author

Choose a reason for hiding this comment

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

In the case of configs, we would not use minimist for handling aliases but we'll handle them ourselves (to avoid polluting the src/index.ts file and get more control over aliases).

In the above block of code, if we encounter an alias set of options, we would set the main config name with the same value, so that we can rely on just the main config name in our scripts.
(Since we are not using minimist for aliases, the main config name will not be set automatically on options.)

@garg3133
Copy link
Member Author

@itsspriyansh Can you also check the failing tests when you get to it. It seems to me that we'd need to mock the src/utils/appium-adb in a bunch of tests.

@itsspriyansh
Copy link
Contributor

itsspriyansh commented Aug 18, 2024

@garg3133 I have tested it for all the conditions and it does handle all the cases perfectly. Looks pretty good to me.

I will accordingly update PR #55 as well.

@garg3133 garg3133 merged commit 8e6d44d into main Aug 18, 2024
2 checks passed
@garg3133 garg3133 deleted the verifyFlagsAndConfigs branch August 18, 2024 12:01
@garg3133
Copy link
Member Author

garg3133 commented Aug 18, 2024

@itsspriyansh We would need to write the tests for verifyOptions function on priority to ensure that all the use cases continue working even after future changes to the function.

@itsspriyansh
Copy link
Contributor

@garg3133 Just wanted to confirm what was the purpose of usageHelp key here? Is it just a placeholder value for the configs or a proper one line description?

@garg3133
Copy link
Member Author

@itsspriyansh It is a replacement for the value property you set before.

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.

2 participants