PR Reviewer Guide 🔍

PR Code Suggestions ✨

-self.service.start()
+try:
+    self.service.start()
+except Exception as e:
+    self.service.stop()
+    raise e
 

+if not executable_path:
+    raise ValueError("Executable path must be provided")
 self.service = Service(executable_path, port=port, log_path=service_log_path)
 

This does fix the problem described in the issue, but this class is problematic in several ways, still

It's hard to write this code when we can't have tests because we aren't managing the drivers.

But I think it needs to look more like:
https://github.com/SeleniumHQ/selenium/blob/trunk/py/selenium/webdriver/wpewebkit/webdriver.py

But I think it needs to look more like

I'm trying to understand the differences between the classes if I make wpewebkit and webkitgtk identical. I removed parameters such as port, executable_path, and log path because there are default parameters in the Service class, and other driver binds similarly do not use them. However, in this case, the two classes become identical, and I'm not sure if I correctly understood what is required in the comment above.

classes in the latest commit

classes after I applied changes described above

It makes sense to me that they are identical if they aren't actually doing anything other than starting/stopping the driver.

@iampopovich the python linter is failing on some of these changes, can you take a look?

@titusfortner Running the script format.sh did not make any corrections, but during the linting stage, isort showed an error. It might be a good idea to add the isort run to the format.sh script.

From root run ./go py:lint

The format script is supposed to work without any additional software installed, but we haven't gotten that figured out for python. If we add the python bit, everyone who doesn't have a python dev environment will get failures. There's an open PR for part of it, we need to dig into it again.