-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
deprecating array of args support in appium service #10972
Conversation
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 it would be more clear if the defragCliArgs
method could be incorporated into formatCliArgs
. Also can we have more tests for the new function?
export function defragCliArgs(args?: AppiumServerArguments | Array<string>) { | ||
if (Array.isArray(args)) { | ||
return args.reduce((acc: KeyValueArgs, currentItem, index, array) => { | ||
const nextItem = array[index + 1] | ||
|
||
if ( | ||
currentItem?.toString().startsWith('--') || | ||
currentItem?.toString().startsWith('-') | ||
) { | ||
const [key, value] = (currentItem ?? '').toString().split('=') | ||
|
||
if (value !== undefined) { | ||
acc[key.replace(/^-+/g, '')] = value | ||
} else if ( | ||
nextItem && | ||
(nextItem?.toString().startsWith('--') || | ||
nextItem?.toString().startsWith('-')) | ||
) { | ||
acc[key.replace(/^-+/g, '')] = true | ||
} else { | ||
acc[key.replace(/^-+/g, '')] = nextItem | ||
} | ||
} | ||
return acc | ||
}, {}) | ||
} | ||
|
||
return args | ||
} |
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.
Let's return early
export function defragCliArgs(args?: AppiumServerArguments | Array<string>) { | |
if (Array.isArray(args)) { | |
return args.reduce((acc: KeyValueArgs, currentItem, index, array) => { | |
const nextItem = array[index + 1] | |
if ( | |
currentItem?.toString().startsWith('--') || | |
currentItem?.toString().startsWith('-') | |
) { | |
const [key, value] = (currentItem ?? '').toString().split('=') | |
if (value !== undefined) { | |
acc[key.replace(/^-+/g, '')] = value | |
} else if ( | |
nextItem && | |
(nextItem?.toString().startsWith('--') || | |
nextItem?.toString().startsWith('-')) | |
) { | |
acc[key.replace(/^-+/g, '')] = true | |
} else { | |
acc[key.replace(/^-+/g, '')] = nextItem | |
} | |
} | |
return acc | |
}, {}) | |
} | |
return args | |
} | |
export function defragCliArgs(args?: AppiumServerArguments | Array<string>) { | |
if (!Array.isArray(args)) { | |
return args | |
} | |
return args.reduce((acc: KeyValueArgs, currentItem, index, array) => { | |
const nextItem = array[index + 1] | |
if ( | |
!currentItem?.toString().startsWith('--') && | |
!currentItem?.toString().startsWith('-') | |
) { | |
return acc | |
} | |
const [key, value] = (currentItem ?? '').toString().split('=') | |
if (value !== undefined) { | |
acc[key.replace(/^-+/g, '')] = value | |
} else if ( | |
nextItem && | |
(nextItem?.toString().startsWith('--') || | |
nextItem?.toString().startsWith('-')) | |
) { | |
acc[key.replace(/^-+/g, '')] = true | |
} else { | |
acc[key.replace(/^-+/g, '')] = nextItem | |
} | |
return acc | |
}, {}) | |
} |
At first, I considered changing the Also as suggested, I have updated the new method and added more tests to check it's functionality. Please share your thoughts on this. |
@tamil777selvan I am a bit confused. It seems to be that with |
Hi @christian-bromann, The documentation explains its capability to accept objects/arrays as inputs for 'args'. Could we consider discontinuing the array support in this scenario? Please share your opinion. |
Yeah, I would suggest that. Given array support never worked, we can safely discontinue it. It is less confusing to only support 1 type here. Would you mind making that change? |
Yeah sure, on it. |
Hi @christian-bromann, I've eliminated the array support for arguments and enforced the type to be an object. Please share your opinion. |
@@ -86,9 +86,9 @@ export const config = { | |||
Map of arguments for the Appium server, passed directly to `appium`. | |||
|
|||
See [the documentation](https://appium.github.io/appium.io/docs/en/writing-running-appium/server-args/index.html) for possible arguments. | |||
The arguments should be supplied in lower camel case, so `--pre-launch true` becomes `preLaunch: true` or passed as an array. | |||
The arguments are supplied in lower camel case. For instance, `debugLogSpacing: true` transforms into `--debug-log-spacing true`, or they can be supplied as outlined in the Appium documentation. |
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.
Boolean flags should not have a value.
The arguments are supplied in lower camel case. For instance, `debugLogSpacing: true` transforms into `--debug-log-spacing true`, or they can be supplied as outlined in the Appium documentation. | |
The arguments are supplied in lower camel case. For instance, `debugLogSpacing: true` transforms into `--debug-log-spacing`, or they can be supplied as outlined in the Appium documentation. |
Can you link Appium documentation to an Appium resource that could be of help 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.
Furthermore, the documentation link is connected in line 88; however, it points to the outdated Appium version. Would it be advisable to directly link to the documentation here on GitHub?
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.
yes, that makes sense
'-p': 1234, | ||
'--relaxed-security': true |
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 don't think we should document such usage and encourage users to write out the full property name.
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.
Understood, but would this apply equally to aliases as well?
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 am not sure if we should support these, I think if someone e.g. sets { p: 1234 }
we can create ['-p', 1234]
out of it. What do you think?
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've observed that this approach isn't applicable to all aliases.
For example: if I have { p: 1234 }
, following the formatting, it would result in ['--p', 1234]
, which Appium would accept.
However, consider a scenario where I provide an address in the alias, such as { a: 'foo' }
. The formatting would lead to ['--a', 'foo']
, triggering an ambiguous error in Appium.
To address this situation, it becomes necessary to either permit the use of full property names in the args or to consider discontinuing the support by updating the documentation. Please share your opinion.
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.
When I look at appium server --help
I get:
appium server [-h] [--address ADDRESS] [--allow-cors]
[--allow-insecure ALLOW_INSECURE]
[--base-path BASE_PATH]
[--callback-address CALLBACK_ADDRESS]
[--callback-port CALLBACK_PORT]
[--debug-log-spacing]
[--default-capabilities DEFAULT_CAPABILITIES]
[--deny-insecure DENY_INSECURE]
[--keep-alive-timeout KEEP_ALIVE_TIMEOUT]
[--local-timezone] [--log LOG]
[--log-filters LOG_FILTERS] [--log-level LOG_LEVEL]
[--log-no-colors] [--log-timestamp]
[--long-stacktrace] [--no-perms-check]
[--nodeconfig NODECONFIG] [--port PORT]
[--relaxed-security] [--session-override]
[--strict-caps] [--tmp TMP] [--trace-dir TRACE_DIR]
[--use-drivers USE_DRIVERS]
[--use-plugins USE_PLUGINS] [--webhook WEBHOOK]
[--shell] [--show-build-info] [--show-config]
[--config CONFIGFILE]
it doesn't seem to advocate for the usage of aliases a lot. I would just suggest to not support those and make this clear in the docs.
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.
From the user experience point of view I feel like this makes the configuration more clear anyway. What do you think?
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.
Certainly, that sounds like a solid plan. I'll integrate those adjustments and make the necessary updates to the documentation.
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.
Hi @christian-bromann, I have implemented all the required modifications. Please review them at your convenience.
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.
Since we are already at this, could we extend the AppiumServerArguments
and actually fill it up with given server properties rather than just having: [capability: string]: any
. The rest looks good to me!
I have included all the parameters and corresponding types utilised by the Appium server CLI. Please review them at your convenience. |
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.
This is awesome, thank you so much 👍
Proposed changes
deprecating array of args support in appium service
Types of changes
Checklist
Further comments
Reviewers: @webdriverio/project-committers