bito-code-review[bot] commented on code in PR #38449:
URL: https://github.com/apache/superset/pull/38449#discussion_r2891385886
##########
superset/utils/webdriver.py:
##########
@@ -399,6 +402,29 @@ def get_screenshot( # pylint: disable=too-many-locals,
too-many-statements # n
class WebDriverSelenium(WebDriverProxy):
+ def __init__(
+ self,
+ driver_type: str,
+ window: WindowSize | None = None,
+ user: User | None = None,
+ ):
+ super().__init__(driver_type, window)
+ self._user = user
+ self._driver: WebDriver | None = None
+
+ def __del__(self) -> None:
+ self._destroy()
+
+ @property
+ def driver(self) -> WebDriver:
+ if not self._driver:
+ self._driver = self._create()
+ assert self._driver # for mypy
+ self._driver.set_window_size(*self._window)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Use of assert statement detected</b></div>
<div id="fix">
Replace `assert` statement with explicit error handling. Use `if not
self._driver: raise RuntimeError(...)` instead of `assert self._driver`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
if not self._driver:
self._driver = self._create()
if not self._driver:
raise RuntimeError("Failed to create WebDriver instance")
self._driver.set_window_size(*self._window)
````
</div>
</details>
</div>
<small><i>Code Review Run #f86fd5</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/utils/webdriver.py:
##########
@@ -399,6 +402,29 @@ def get_screenshot( # pylint: disable=too-many-locals,
too-many-statements # n
class WebDriverSelenium(WebDriverProxy):
+ def __init__(
+ self,
+ driver_type: str,
+ window: WindowSize | None = None,
+ user: User | None = None,
+ ):
+ super().__init__(driver_type, window)
+ self._user = user
+ self._driver: WebDriver | None = None
+
+ def __del__(self) -> None:
+ self._destroy()
+
+ @property
+ def driver(self) -> WebDriver:
+ if not self._driver:
+ self._driver = self._create()
+ assert self._driver # for mypy
+ self._driver.set_window_size(*self._window)
+ if self._user:
+ self._auth(self._user)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Authentication bypassed in driver property</b></div>
<div id="fix">
The driver property calls _auth but ignores its return value, and _auth
creates a new driver unnecessarily. This results in unauthenticated drivers for
screenshots when user is provided. The authentication logic is bypassed.
</div>
</div>
<small><i>Code Review Run #f86fd5</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
