Skip to content

[py] implementation of new feature-12760, setting service_args through getters/setters #12767

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.

Sign up for GitHub

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

Closed
wants to merge 11 commits into from
Closed

[py] implementation of new feature-12760, setting service_args through getters/setters #12767

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
70ed717
[py] implementation of setting service_args through getters/setters -…
sandeepsuryaprasad Sep 18, 2023
9cce058
fixed typo
sandeepsuryaprasad Sep 18, 2023
935fdec
removed un-necessary print statement
sandeepsuryaprasad Sep 18, 2023
d96756e
reverted ie log levels and minimum supported python version to 3.9
sandeepsuryaprasad Sep 18, 2023
89af175
commiting .idea/misc.xml
sandeepsuryaprasad Sep 18, 2023
3ae9054
resolved merge conflicts
sandeepsuryaprasad Sep 18, 2023
e765368
reveted minimum py version to 3.9 in python.iml
sandeepsuryaprasad Sep 18, 2023
d257645
cleanup
sandeepsuryaprasad Sep 18, 2023
8702e03
fixed typo
sandeepsuryaprasad Sep 18, 2023
093e3df
incorporated review comments
sandeepsuryaprasad Sep 28, 2023
b5d8ba5
fixed typo
sandeepsuryaprasad Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions .idea/misc.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion py/python.iml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,4 @@
<option name="useAlternativeWorkingDir" value="false" />
<option name="workingDirSelection" value="&lt;MODULE&gt;" />
</component>
</module>
</module>
18 changes: 15 additions & 3 deletions py/selenium/webdriver/chromium/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,12 @@ def __init__(
env: typing.Optional[typing.Mapping[str, str]] = None,
**kwargs,
) -> None:
self.service_args = service_args or []
if service_args is None:
service_args = []
self._service_args = service_args

if isinstance(log_output, str):
self.service_args.append(f"--log-path={log_output}")
self._service_args.append(f"--log-path={log_output}")
self.log_output = None
else:
self.log_output = log_output
Expand All @@ -56,5 +58,15 @@ def __init__(
**kwargs,
)

@property
def service_args(self):
Copy link
Member

Choose a reason for hiding this comment

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

be nice to add typing to things we are adding, feel free to ignore tho as theres plenty of general typing work to do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@symonk thanks for your review comments. I have updated the code.

return self._service_args

@service_args.setter
def service_args(self, value):
if not isinstance(value, list):
raise TypeError("service args must be a list")
self._service_args.extend(value)

def command_line_args(self) -> typing.List[str]:
return [f"--port={self.port}"] + self.service_args
return [f"--port={self.port}"] + self._service_args
19 changes: 15 additions & 4 deletions py/selenium/webdriver/edge/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,21 +44,32 @@ def __init__(
env: typing.Optional[typing.Mapping[str, str]] = None,
**kwargs,
) -> None:
self.service_args = service_args or []
if service_args is None:
service_args = []
self._service_args = service_args

if verbose:
warnings.warn(
"verbose=True is deprecated. Use `service_args=['--verbose', ...]` instead.",
DeprecationWarning,
stacklevel=2,
)
self.service_args.append("--verbose")

self._service_args.append("--verbose")
super().__init__(
executable_path=executable_path,
port=port,
service_args=service_args,
service_args=self.service_args,
log_output=log_output,
env=env,
**kwargs,
)

@property
def service_args(self):
return self._service_args

@service_args.setter
def service_args(self, value):
if not isinstance(value, list):
raise TypeError("service args must be a list")
self._service_args.extend(value)
27 changes: 19 additions & 8 deletions py/selenium/webdriver/firefox/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
# specific language governing permissions and limitations
# under the License.
import typing
from typing import List

from selenium.types import SubprocessStdAlias
from selenium.webdriver.common import service
Expand All @@ -42,7 +41,14 @@ def __init__(
env: typing.Optional[typing.Mapping[str, str]] = None,
**kwargs,
) -> None:
self.service_args = service_args or []
if service_args is None:
service_args = []
self._service_args = service_args

# Set a port for CDP
if "--connect-existing" not in self._service_args:
self._service_args.append("--websocket-port")
self._service_args.append(f"{utils.free_port()}")

super().__init__(
executable=executable_path,
Expand All @@ -52,10 +58,15 @@ def __init__(
**kwargs,
)

# Set a port for CDP
if "--connect-existing" not in self.service_args:
self.service_args.append("--websocket-port")
self.service_args.append(f"{utils.free_port()}")
@property
def service_args(self):
return self._service_args

@service_args.setter
def service_args(self, value):
if not isinstance(value, list):
raise TypeError("service args must be a list")
self._service_args.extend(value)

def command_line_args(self) -> List[str]:
return ["--port", f"{self.port}"] + self.service_args
def command_line_args(self) -> typing.List[str]:
return ["--port", f"{self.port}"] + self._service_args
26 changes: 19 additions & 7 deletions py/selenium/webdriver/ie/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
# specific language governing permissions and limitations
# under the License.
import typing
from typing import List

from selenium.types import SubprocessStdAlias
from selenium.webdriver.common import service
Expand Down Expand Up @@ -45,18 +44,31 @@ def __init__(
- log_output: (Optional) int representation of STDOUT/DEVNULL, any IO instance or String path to file.
Default is "stdout".
"""
self.service_args = service_args or []
if service_args is None:
service_args = []
self._service_args = service_args

if host:
self.service_args.append(f"--host={host}")
if log_level:
self.service_args.append(f"--log-level={log_level}")
self._service_args.append(f"--host={host}")

if log_level:
self._service_args.append(f"--log-level={log_level}")
super().__init__(
executable_path,
port=port,
log_output=log_output,
**kwargs,
)

def command_line_args(self) -> List[str]:
return [f"--port={self.port}"] + self.service_args
@property
def service_args(self):
return self._service_args

@service_args.setter
def service_args(self, value):
if not isinstance(value, list):
raise TypeError("Service args must be a list")
self._service_args.extend(value)

def command_line_args(self) -> typing.List[str]:
return [f"--port={self.port}"] + self._service_args
21 changes: 17 additions & 4 deletions py/selenium/webdriver/safari/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,20 +42,30 @@ def __init__(
reuse_service=False,
**kwargs,
) -> None:
self.service_args = service_args or []
if service_args is None:
service_args = []
self._service_args = service_args

if quiet is not None:
warnings.warn("quiet is no longer needed to supress output", DeprecationWarning, stacklevel=2)
self.reuse_service = reuse_service

self._reuse_service = reuse_service
super().__init__(
executable=executable_path,
port=port,
env=env,
**kwargs,
)

def command_line_args(self) -> typing.List[str]:
return ["-p", f"{self.port}"] + self.service_args
@property
def service_args(self):
return self._service_args

@service_args.setter
def service_args(self, value):
if not isinstance(value, list):
raise TypeError("service args must be a list")
self._service_args.extend(value)
Copy link

Choose a reason for hiding this comment

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

It might be just me, but the setter seems more like an append rather than replace/set the value.

Is this the intended behaviour? Setters for me usually replace the existing value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

every time when assignment happens to service_args I am not replacing the old args, rather I am updating the existing contents of the list.
e.g.

service = Service(service_args = ["--log", "debug"])
service.service_args = ["--port", 1234]
print(service.service_args)
# prints 
["--log", "debug", "--port", 1234]

@titusfortner , should this be the intended behaviour? or should the old list be replaced with new one every time an assignment happens to service_args?

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 know the Python convention, but it does feel a little weird to append to something existing with an assignment operator. (They way Ruby does it is so much better 😉). I lean toward replacing instead of adding. @isaulv what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

This should definitely not be a setter as that creates the expectation that it is setting the value that you pass in. It is not intuitive to users that it will extend. We either need a different method name or we should see about closing this PR.


@property
def service_url(self) -> str:
Expand All @@ -71,3 +81,6 @@ def reuse_service(self, reuse: bool) -> None:
if not isinstance(reuse, bool):
raise TypeError("reuse must be a boolean")
self._reuse_service = reuse

def command_line_args(self) -> typing.List[str]:
return ["-p", f"{self.port}"] + self._service_args
17 changes: 15 additions & 2 deletions py/selenium/webdriver/webkitgtk/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ def __init__(
env: typing.Optional[typing.Mapping[str, str]] = None,
**kwargs,
):
self.service_args = service_args or []
if service_args is None:
service_args = []
self._service_args = service_args

log_file = open(log_path, "wb") if log_path else None
super().__init__(
executable=executable_path,
Expand All @@ -51,5 +54,15 @@ def __init__(
**kwargs,
) # type: ignore

@property
def service_args(self):
return self._service_args

@service_args.setter
def service_args(self, value):
if not isinstance(value, list):
raise TypeError("service args must be a list")
self._service_args.extend(value)

def command_line_args(self) -> typing.List[str]:
return ["-p", f"{self.port}"] + self.service_args
return ["-p", f"{self.port}"] + self._service_args
17 changes: 15 additions & 2 deletions py/selenium/webdriver/wpewebkit/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ def __init__(
env: typing.Optional[typing.Mapping[str, str]] = None,
**kwargs,
):
self.service_args = service_args or []
if service_args is None:
service_args = []
self._service_args = service_args

log_file = open(log_path, "wb") if log_path else None
super().__init__(
executable=executable_path,
Expand All @@ -51,5 +54,15 @@ def __init__(
**kwargs,
) # type: ignore

@property
def service_args(self):
return self._service_args

@service_args.setter
def service_args(self, value):
if not isinstance(value, list):
raise TypeError("service args must be a list")
self._service_args.extend(value)

def command_line_args(self) -> typing.List[str]:
return ["-p", f"{self.port}"] + self.service_args
return ["-p", f"{self.port}"] + self._service_args