Rename browser to driver by aaltat · Pull Request #976 · robotframework/SeleniumLibrary

aaltat · 2017-10-30T21:55:52Z

To do:

fix browsermanagement.py, because it says browser in many places where it should say driver
deprecate method self.browser

aaltat · 2017-10-31T09:44:00Z

But it definitely looks like my rebase with master did not go smoothly. Have to take a look later.

pekkaklarck · 2017-10-31T10:07:06Z

I'd still use webdriver instead of driver as the attribute name, but it seems everyone else likes the latter so let's use that. I'd still use use WebDriverCache and not DriverCache, though. More importantly, the type of the attribute (i.e. property) should be specified in the docstring. That makes the attribute type unambiguous for users and also helps IDEs.

aaltat · 2017-10-31T21:55:58Z

The windowmanager.py needs some refactoring, so that it does not pass the driver as argument but uses the self.driver instead

pekkaklarck · 2017-10-31T22:20:11Z

src/SeleniumLibrary/__init__.py

            WindowKeywords(self)
        ]
-        self._browsers = BrowserCache()
+        self._webdrivers = WebDriverCache()


This should be _drivers to match driver.

pekkaklarck · 2017-10-31T22:21:01Z

src/SeleniumLibrary/base/context.py

    @property
-    def browsers(self):
-        return self.ctx._browsers
+    def webdrivers(self):


This should be drivers to match driver.

pekkaklarck · 2017-10-31T22:24:43Z

src/SeleniumLibrary/keywords/browsermanagement.py

        self.ctx.speed = timestr_to_secs(value)
-        for browser in self.browsers.browsers:
-            self._monkey_patch_speed(browser)
+        for driver in self.webdrivers.drivers:


Isn't ConnectionCache iterable? If it is, this could be just for driver in self.drivers. If it isn't, it should be changed!

It should be iterable, but changes the code to only iterate drivers which are not closed.

pekkaklarck · 2017-11-06T21:36:41Z

src/SeleniumLibrary/__init__.py

            self._running_on_failure_keyword = False

+    @property
+    def driver(self):


. missing at the end.

pekkaklarck · 2017-11-06T21:38:06Z

src/SeleniumLibrary/__init__.py

+        :rtype: selenium.webdriver.remote.webdriver.WebDriver
+        :raises RuntimeError if driver is not created
+        """
+        if not self._drivers.current:


Strange error message. Perhaps this should still be No browser is open.? Also, should we use a dedicated exception?

Yes, browser is better.

It's strange exception, but that is how the original code worked. Dedicated exception would be better, would NoBrowser be good?

Perhaps NoOpenBrowser or NoBrowserOpen?

pekkaklarck · 2017-11-06T21:48:50Z

src/SeleniumLibrary/utils/webdrivercache.py

-            if browser not in self._closed:
-                open_browsers.append(browser)
-        return open_browsers
+    def get_open_drivers(self):


Open driver sounds a bit odd. Should this perhaps be get_active_browsers()? Or perhaps property active_browsers?

Did go with active_drivers

pekkaklarck

Looks great. Added few line notes, though.

pekkaklarck · 2017-11-07T22:08:34Z

src/SeleniumLibrary/utils/webdrivercache.py


    def __init__(self):
-        ConnectionCache.__init__(self, no_current_msg='No current browser')
+        ConnectionCache.__init__(self, no_current_msg='No current driver')


This should still say browser. And have . at the end.

If that message is ever used, it's used by ConnectionCache as a message for RuntimeError. We have our own NoOpenBrowser now, but there's no good API to use that here. We could set self._no_current to an object using our exception (and it could extend NoConnection) but I'm not sure is that worth the effort.

I did change to driver -> browser.

If the start seeing the RuntimeErrorin real life, lets then decide what we should do.

pekkaklarck · 2017-11-07T22:09:39Z

src/SeleniumLibrary/__init__.py

+        :rtype: selenium.webdriver.remote.webdriver.WebDriver
+        :raises RuntimeError if driver is not created
+        """
+        if not self._drivers.current:


Exception type not updated in the doc string above.

This error message could have . at the end.

Also created public API browser for RC1 to support early adopters.

Contains changes in the internal variables and method names.

The WindowManager used to pass driver between methods. Now the methods use self.driver, instead of passing the diver as argument.

pekkaklarck · 2017-11-08T20:49:07Z

src/SeleniumLibrary/__init__.py

+    def driver(self):
+        """Current active driver.
+
+        :rtype: selenium.webdriver.remote.webdriver.WebDriver


Thought it would raise NoOpenBrowser....

pekkaklarck reviewed Nov 5, 2017

View reviewed changes

aaltat force-pushed the rename_browser_to_driver_2 branch from d212b8e to 476cb6f Compare November 6, 2017 20:47

pekkaklarck reviewed Nov 6, 2017

View reviewed changes

pekkaklarck approved these changes Nov 6, 2017

View reviewed changes

aaltat force-pushed the rename_browser_to_driver_2 branch from 476cb6f to 2faa957 Compare November 7, 2017 21:39

pekkaklarck reviewed Nov 7, 2017

View reviewed changes

aaltat added 11 commits November 8, 2017 22:11

rename browsercache.py to drivercache.py

dad592f

Fixed DriverCache internal from browser to driver

83c1087

Changed in code browser(s) to driver(s)

cbe4ec1

Fixing code to pass tests

d069dfd

Renamed DriverCache to WebDriverCache

97ebf3a

Added documentation for driver public API.

5d5bf70

Also created public API browser for RC1 to support early adopters.

Refactoring BrowserManagementKeywords browser -> driver

d860a05

Contains changes in the internal variables and method names.

Refactor WindowManager to use self.driver

7209328

The WindowManager used to pass driver between methods. Now the methods use self.driver, instead of passing the diver as argument.

Updated based on review comments

250e261

Small adjustments based on comments.

61d011f

Fixed doc string and error message in cache

c8a7c2f

aaltat force-pushed the rename_browser_to_driver_2 branch from f24f1ca to c8a7c2f Compare November 8, 2017 20:13

aaltat added 2 commits November 8, 2017 22:34

Fixed tests because error message was changed

fbca5d2

Fixes sphinx doc definition

7a24e4b

pekkaklarck reviewed Nov 8, 2017

View reviewed changes

Fixes agains the doc string

4c8148a

pekkaklarck merged commit dc875cf into robotframework:master Nov 8, 2017

aaltat deleted the rename_browser_to_driver_2 branch November 14, 2017 20:40

Conversation

aaltat commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaltat commented Oct 31, 2017

Uh oh!

pekkaklarck commented Oct 31, 2017

Uh oh!

aaltat commented Oct 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pekkaklarck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aaltat commented Oct 30, 2017 •

edited

Loading