-
Notifications
You must be signed in to change notification settings - Fork 568
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
feat: define AppiumClientConfig #1070
base: master
Are you sure you want to change the base?
Conversation
When will this PR be merged? |
appium/webdriver/webdriver.py
Outdated
options: Union[AppiumOptions, List[AppiumOptions], None] = None, | ||
client_config: Optional[ClientConfig] = None, | ||
client_config: Optional[AppiumClientConfig] = None, | ||
): | ||
if isinstance(command_executor, str): |
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 extract this logic into a separate method, so super class init would be the very first call there?
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.
super class init would be the very first call there?
Actually we could by modifying here like below:
def __init__(
self,
command_executor: AppiumConnection,
client_config: AppiumClientConfig,
extensions: Optional[List['WebDriver']] = None,
file_detector: Optional[FileDetector] = None,
options: Union[AppiumOptions, List[AppiumOptions], None] = None,
):
Then, a new method will help to build client_config and command_executor.
To avoid handling string of command_executor
in selenium, which uses ClientConfig
, I have added these lines to keep using AppiumClientConfig
as appium python client for such a string case (to keep providing similar syntax to webdriver.Remote for selenium and appium)
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.
Pull Request Overview
This PR introduces the new AppiumClientConfig to centralize client configuration while deprecating legacy direct connection arguments, which is a breaking change. Key changes include:
- New file appium/webdriver/client_config.py defining the AppiumClientConfig.
- Updates in tests and migration documentation to use the new client_config parameter.
- Modifications in WebDriver initialization to integrate AppiumClientConfig for command executor creation.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
appium/webdriver/client_config.py | Introduces the new AppiumClientConfig class. |
test/unit/webdriver/webdriver_test.py | Updates tests to use client_config instead of direct_connection. |
README.md | Provides migration guide and updates examples for new config usage. |
appium/webdriver/webdriver.py | Updates WebDriver and connection logic to integrate AppiumClientConfig. |
Comments suppressed due to low confidence (1)
appium/webdriver/client_config.py:28
- Consider rephrasing 'If enables' to 'If enabled' for grammatical correctness in the argument description.
direct_connection: If enables [directConnect](https://github.com/appium/python-client?tab=readme-ov-file#direct-connect-urls) feature.
Co-authored-by: Copilot <[email protected]>
SERVER_URL_BASE = 'http://127.0.0.1:4723' | ||
# before | ||
driver = webdriver.Remote( | ||
SERVER_URL_BASE, |
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 would still prefer to keep the url
positional argument
I believe most of the clients have the remote client arguments defined as (url, options), so this change would require all of them to change the code, which is a major inconvenience.
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.
upd: I can observe the below code supports such feature, so we just need to mention that in documents
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.
Sounds good.
The ClinetConfig requires remote_server_addr
https://github.com/SeleniumHQ/selenium/blob/dbf3daef5519b0a35a8408510854ec7766455c66/py/selenium/webdriver/remote/client_config.py#L80C9-L80C27 so current example is to reduce adding remote_server_addr
to both, but actually current one will confuse users.
I'll add this.
appium/webdriver/webdriver.py
Outdated
@@ -174,6 +174,27 @@ def add_command(self) -> Tuple[str, str]: | |||
raise NotImplementedError() | |||
|
|||
|
|||
def _get_remote_connection_and_client_config( | |||
command_executor: Union[str, AppiumConnection], client_config: Optional[AppiumClientConfig] |
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.
should the optional value be assigned to None by default?
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.
Either is fine. Just wanted to let the caller explicitly set both
@@ -237,8 +246,8 @@ def __init__( # noqa: PLR0913 | |||
|
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 believe if hasattr(self, 'command_executor')
would always return True
BREAKING:
Let's move the client configuration stuff to AppiumClientConfig only. This will be a major version up.