-
Notifications
You must be signed in to change notification settings - Fork 73
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
Improve GT.save()
usability
#499
Merged
Merged
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
cccee66
Improve `GT.save()` usability
jrycw f23661a
Isolate web driver preparation logic from `GT.save()`
jrycw 79f5461
Add `quit()` to `_NoOpDriverCtx`
jrycw 5abca23
Try to replace `_NoOpDriverCtx` with `no_op_callable()`
jrycw 1b599da
Add `from __future__ import annotations` to `_utils_selenium.py`
jrycw e0b6365
Introduce `cls_driver` and `cls_wd_options` to WebDriver
jrycw 73a7c86
Fix wrong `cls_driver` for `_SafariWebDriver`
jrycw 4c6d965
Fix the type hint for `debug_port` in `_BaseWebDriver`
jrycw d319ce1
Add `test__utils_selenium.py`
jrycw a2ee377
Remove `**params` from `GT.save()`
jrycw 23f12cb
Restore the comment about using `PIL` for converting tables into diff…
jrycw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
from __future__ import annotations | ||
|
||
from types import TracebackType | ||
from typing import Literal | ||
from typing_extensions import TypeAlias | ||
from selenium import webdriver | ||
|
||
# Create a list of all selenium webdrivers | ||
WebDrivers: TypeAlias = Literal[ | ||
"chrome", | ||
"firefox", | ||
"safari", | ||
"edge", | ||
] | ||
|
||
|
||
class _BaseWebDriver: | ||
|
||
def __init__(self, debug_port: int | None = None): | ||
self.debug_port = debug_port | ||
self.wd_options = self.cls_wd_options() | ||
self.add_arguments() | ||
self.driver = self.cls_driver(self.wd_options) | ||
|
||
def add_arguments(self): ... | ||
|
||
def __enter__(self) -> WebDrivers | webdriver.Remote: | ||
return self.driver | ||
|
||
def __exit__( | ||
self, | ||
exc_type: type[BaseException] | None, | ||
exc_value: BaseException | None, | ||
traceback: TracebackType | None, | ||
) -> bool | None: | ||
self.driver.quit() | ||
|
||
|
||
class _ChromeWebDriver(_BaseWebDriver): | ||
cls_driver = webdriver.Chrome | ||
cls_wd_options = webdriver.ChromeOptions | ||
|
||
def add_arguments(self): | ||
self.wd_options.add_argument("--headless=new") | ||
if self.debug_port is not None: | ||
self.wd_options.add_argument(f"--remote-debugging-port={self.debug_port}") | ||
|
||
|
||
class _SafariWebDriver(_BaseWebDriver): | ||
cls_driver = webdriver.Safari | ||
cls_wd_options = webdriver.SafariOptions | ||
|
||
|
||
class _FirefoxWebDriver(_BaseWebDriver): | ||
cls_driver = webdriver.Firefox | ||
cls_wd_options = webdriver.FirefoxOptions | ||
|
||
def add_arguments(self): | ||
self.wd_options.add_argument("--headless") | ||
if self.debug_port is not None: | ||
self.wd_options.add_argument(f"--start-debugger-server {self.debug_port}") | ||
|
||
|
||
class _EdgeWebDriver(_BaseWebDriver): | ||
cls_driver = webdriver.Edge | ||
cls_wd_options = webdriver.EdgeOptions | ||
|
||
def add_arguments(self): | ||
self.wd_options.add_argument("--headless") | ||
|
||
|
||
def no_op_callable(web_driver: webdriver.Remote): | ||
def wrapper(*args, **kwargs): | ||
return web_driver | ||
|
||
return wrapper | ||
|
||
|
||
def _get_web_driver(web_driver: WebDrivers | webdriver.Remote): | ||
if isinstance(web_driver, webdriver.Remote): | ||
return no_op_callable(web_driver) | ||
elif web_driver == "chrome": | ||
return _ChromeWebDriver | ||
elif web_driver == "safari": | ||
return _SafariWebDriver | ||
elif web_driver == "firefox": | ||
return _FirefoxWebDriver | ||
elif web_driver == "edge": | ||
return _EdgeWebDriver | ||
else: | ||
raise ValueError(f"Unsupported web driver: {web_driver}") |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
import pytest | ||
|
||
from great_tables._utils_selenium import ( | ||
_get_web_driver, | ||
no_op_callable, | ||
_ChromeWebDriver, | ||
_SafariWebDriver, | ||
_FirefoxWebDriver, | ||
_EdgeWebDriver, | ||
) | ||
|
||
|
||
def test_no_op_callable(): | ||
""" | ||
The test should cover the scenario of obtaining a remote driver in `_get_web_driver`. | ||
""" | ||
fake_input = object() | ||
f = no_op_callable(fake_input) | ||
assert f(1, x="x") is fake_input | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"web_driver,Driver", | ||
[ | ||
("chrome", _ChromeWebDriver), | ||
("safari", _SafariWebDriver), | ||
("firefox", _FirefoxWebDriver), | ||
("edge", _EdgeWebDriver), | ||
], | ||
) | ||
def test_get_web_driver(web_driver, Driver): | ||
assert _get_web_driver(web_driver) is Driver | ||
|
||
|
||
def test_get_web_driver_raise(): | ||
fake_web_driver = "fake_web_driver" | ||
with pytest.raises(ValueError) as exc_info: | ||
_get_web_driver(fake_web_driver) | ||
assert exc_info.value.args[0] == f"Unsupported web driver: {fake_web_driver}" |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is it okay if we remove the
**params
piece of this PR? I'm onboard with everything else. Because**params
only applies to when we go from a.png
-> something else, it exposes PIL as part of the user API.If we leave it out, we can swap out PIL down the road. As an alternative could we tell users that we're using Pillow? We could even tell them about PIL open and save if needed (so they can go from .png -> anything PIL supports on their own if more customization is needed).
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.
Thanks for the explanation. I agree that we should remove
**params
, makingPillow
an internal dependency rather than a public one.Regarding hints, I believe that diligent readers likely already recognize we use
Pillow
under the hood and would consult its documentation for further customization if needed.This aligns with our goal of providing a user-friendly way to save generated tables rather than focusing on creating highly customized table figures in various formats🤔.