From e69c31e51f56e1f26647114a4c3f5c228e47f536 Mon Sep 17 00:00:00 2001 From: Ben Mares <15216687+maresb@users.noreply.github.com> Date: Fri, 23 Apr 2021 18:21:43 +0200 Subject: [PATCH 1/5] Handle pathlib.Path in write_image --- packages/python/plotly/plotly/io/_kaleido.py | 44 +++++-- .../test_kaleido/test_kaleido.py | 116 +++++++++++++----- .../plotly/tests/test_orca/test_to_image.py | 22 +++- 3 files changed, 142 insertions(+), 40 deletions(-) diff --git a/packages/python/plotly/plotly/io/_kaleido.py b/packages/python/plotly/plotly/io/_kaleido.py index e56d095d977..dcbe7f8ea72 100644 --- a/packages/python/plotly/plotly/io/_kaleido.py +++ b/packages/python/plotly/plotly/io/_kaleido.py @@ -2,6 +2,7 @@ from six import string_types import os import json +from pathlib import Path, PurePath import plotly from plotly.io._utils import validate_coerce_fig_to_dict @@ -169,7 +170,7 @@ def write_image( file: str or writeable A string representing a local file path or a writeable object - (e.g. an open file descriptor) + (e.g. a pathlib.Path object or an open file descriptor) format: str or None The desired image format. One of @@ -228,14 +229,23 @@ def write_image( ------- None """ - # Check if file is a string - # ------------------------- - file_is_str = isinstance(file, string_types) + # Try to cast `file` as a pathlib object `path`. + # ---------------------------------------------- + if isinstance(file, string_types): + # Use the standard pathlib constructor to make a pathlib object. + path = Path(file) + elif isinstance(file, PurePath): # PurePath is the most general pathlib object. + # `file` is already a pathlib object. + path = file + else: + # We could not make a pathlib object out of file. Either `file` is an open file + # descriptor with a `write()` method or it's an invalid object. + path = None # Infer format if not specified # ----------------------------- - if file_is_str and format is None: - _, ext = os.path.splitext(file) + if path is not None and format is None: + ext = path.suffix if ext: format = ext.lstrip(".") else: @@ -267,11 +277,25 @@ def write_image( # Open file # --------- - if file_is_str: - with open(file, "wb") as f: - f.write(img_data) + if path is None: + # We previously failed to make sense of `file` as a pathlib object. + # Attempt to write to `file` as an open file descriptor. + try: + file.write(img_data) + return + except AttributeError: + pass + raise ValueError( + """ +The 'file' argument '{file}' is not a string, pathlib.Path object, or file descriptor. +""".format( + file=file + ) + ) else: - file.write(img_data) + # We previously succeeded in interpreting `file` as a pathlib object. + # Now we can use `write_bytes()`. + path.write_bytes(img_data) def full_figure_for_development(fig, warn=True, as_dict=False): diff --git a/packages/python/plotly/plotly/tests/test_optional/test_kaleido/test_kaleido.py b/packages/python/plotly/plotly/tests/test_optional/test_kaleido/test_kaleido.py index 4843ae46ffd..b6ba5ee11f8 100644 --- a/packages/python/plotly/plotly/tests/test_optional/test_kaleido/test_kaleido.py +++ b/packages/python/plotly/plotly/tests/test_optional/test_kaleido/test_kaleido.py @@ -1,16 +1,56 @@ import plotly.io as pio import plotly.io.kaleido -import sys from contextlib import contextmanager - -if sys.version_info >= (3, 3): - from unittest.mock import Mock -else: - from mock import Mock +from io import BytesIO +from pathlib import Path +from unittest.mock import Mock fig = {"layout": {"title": {"text": "figure title"}}} +def make_writeable_mocks(): + """Produce some mocks which we will use for testing the `write_image()` function. + + These mocks should be passed as the `file=` argument to `write_image()`. + + The tests should verify that the method specified in the `active_write_function` + attribute is called once, and that scope.transform is called with the `format=` + argument specified by the `.expected_format` attribute. + + In total we provide two mocks: one for a writable file descriptor, and other for a + pathlib.Path object. + """ + + # Part 1: A mock for a file descriptor + # ------------------------------------ + mock_file_descriptor = Mock() + + # A file descriptor has no write_bytes method, unlike a pathlib Path. + del mock_file_descriptor.write_bytes + + # The expected write method for a file descriptor is .write + mock_file_descriptor.active_write_function = mock_file_descriptor.write + + # Since there is no filename, there should be no format detected. + mock_file_descriptor.expected_format = None + + # Part 2: A mock for a pathlib path + # --------------------------------- + mock_pathlib_path = Mock(spec=Path) + + # A pathlib Path object has no write method, unlike a file descriptor. + del mock_pathlib_path.write + + # The expected write method for a pathlib Path is .write_bytes + mock_pathlib_path.active_write_function = mock_pathlib_path.write_bytes + + # Mock a path with PNG suffix + mock_pathlib_path.suffix = ".png" + mock_pathlib_path.expected_format = "png" + + return mock_file_descriptor, mock_pathlib_path + + @contextmanager def mocked_scope(): # Code to acquire resource, e.g.: @@ -44,15 +84,19 @@ def test_kaleido_engine_to_image(): def test_kaleido_engine_write_image(): - writeable_mock = Mock() - with mocked_scope() as scope: - pio.write_image(fig, writeable_mock, engine="kaleido", validate=False) + for writeable_mock in make_writeable_mocks(): + with mocked_scope() as scope: + pio.write_image(fig, writeable_mock, engine="kaleido", validate=False) - scope.transform.assert_called_with( - fig, format=None, width=None, height=None, scale=None - ) + scope.transform.assert_called_with( + fig, + format=writeable_mock.expected_format, + width=None, + height=None, + scale=None, + ) - assert writeable_mock.write.call_count == 1 + assert writeable_mock.active_write_function.call_count == 1 def test_kaleido_engine_to_image_kwargs(): @@ -73,24 +117,24 @@ def test_kaleido_engine_to_image_kwargs(): def test_kaleido_engine_write_image_kwargs(): - writeable_mock = Mock() - with mocked_scope() as scope: - pio.write_image( - fig, - writeable_mock, - format="jpg", - width=700, - height=600, - scale=2, - engine="kaleido", - validate=False, + for writeable_mock in make_writeable_mocks(): + with mocked_scope() as scope: + pio.write_image( + fig, + writeable_mock, + format="jpg", + width=700, + height=600, + scale=2, + engine="kaleido", + validate=False, + ) + + scope.transform.assert_called_with( + fig, format="jpg", width=700, height=600, scale=2 ) - scope.transform.assert_called_with( - fig, format="jpg", width=700, height=600, scale=2 - ) - - assert writeable_mock.write.call_count == 1 + assert writeable_mock.active_write_function.call_count == 1 def test_image_renderer(): @@ -105,3 +149,17 @@ def test_image_renderer(): height=renderer.height, scale=renderer.scale, ) + + +def test_bytesio(): + """Verify that writing to a BytesIO object contains the same data as to_image(). + + The goal of this test is to ensure that Plotly correctly handles a writable buffer + which doesn't correspond to a filesystem path. + """ + bio = BytesIO() + pio.write_image(fig, bio, format="jpg", engine="kaleido", validate=False) + bio.seek(0) # Rewind to the beginning of the buffer, otherwise read() returns b''. + bio_bytes = bio.read() + to_image_bytes = pio.to_image(fig, format="jpg", engine="kaleido", validate=False) + assert bio_bytes == to_image_bytes diff --git a/packages/python/plotly/plotly/tests/test_orca/test_to_image.py b/packages/python/plotly/plotly/tests/test_orca/test_to_image.py index a4f88b203a4..5343c29456a 100644 --- a/packages/python/plotly/plotly/tests/test_orca/test_to_image.py +++ b/packages/python/plotly/plotly/tests/test_orca/test_to_image.py @@ -6,6 +6,7 @@ import pytest import sys import pandas as pd +from io import BytesIO if sys.version_info >= (3, 3): from unittest.mock import MagicMock @@ -234,7 +235,12 @@ def test_write_image_writeable(fig1, format): fig1, mock_file, format=format, width=700, height=500, engine="orca" ) - mock_file.write.assert_called_once_with(expected_bytes) + if mock_file.write_bytes.called: + mock_file.write_bytes.assert_called_once_with(expected_bytes) + elif mock_file.write.called: + mock_file.write.assert_called_once_with(expected_bytes) + else: + assert "Neither write nor write_bytes was called." def test_write_image_string_format_inference(fig1, format): @@ -347,3 +353,17 @@ def test_invalid_figure_json(): ) assert "400: invalid or malformed request syntax" in str(err.value) + + +def test_bytesio(fig1): + """Verify that writing to a BytesIO object contains the same data as to_image(). + + The goal of this test is to ensure that Plotly correctly handles a writable buffer + which doesn't correspond to a filesystem path. + """ + bio = BytesIO() + pio.write_image(fig1, bio, format="jpg", validate=False) + bio.seek(0) + bio_bytes = bio.read() + to_image_bytes = pio.to_image(fig1, format="jpg", validate=False) + assert bio_bytes == to_image_bytes From 4dbfb374de9c1d59cb0f7e3a5b0dfb765456fc4e Mon Sep 17 00:00:00 2001 From: Ben Mares <15216687+maresb@users.noreply.github.com> Date: Fri, 23 Apr 2021 20:27:18 +0200 Subject: [PATCH 2/5] Allow pathlib.Path in write_html --- .../python/plotly/plotly/basedatatypes.py | 4 +- packages/python/plotly/plotly/io/_html.py | 32 +++++++---- packages/python/plotly/plotly/io/_kaleido.py | 10 ++-- .../plotly/tests/test_io/test_pathlib.py | 57 +++++++++++++++++++ 4 files changed, 84 insertions(+), 19 deletions(-) create mode 100644 packages/python/plotly/plotly/tests/test_io/test_pathlib.py diff --git a/packages/python/plotly/plotly/basedatatypes.py b/packages/python/plotly/plotly/basedatatypes.py index 7954dea37f6..e39fe0fdcce 100644 --- a/packages/python/plotly/plotly/basedatatypes.py +++ b/packages/python/plotly/plotly/basedatatypes.py @@ -3578,7 +3578,7 @@ def write_html(self, *args, **kwargs): ---------- file: str or writeable A string representing a local file path or a writeable object - (e.g. an open file descriptor) + (e.g. a pathlib.Path object or an open file descriptor) config: dict or None (default None) Plotly.js figure config options auto_play: bool (default=True) @@ -3751,7 +3751,7 @@ def write_image(self, *args, **kwargs): ---------- file: str or writeable A string representing a local file path or a writeable object - (e.g. an open file descriptor) + (e.g. a pathlib.Path object or an open file descriptor) format: str or None The desired image format. One of diff --git a/packages/python/plotly/plotly/io/_html.py b/packages/python/plotly/plotly/io/_html.py index cb90de32f98..666ea2e7f95 100644 --- a/packages/python/plotly/plotly/io/_html.py +++ b/packages/python/plotly/plotly/io/_html.py @@ -1,6 +1,7 @@ import uuid import json import os +from pathlib import Path import webbrowser import six @@ -401,7 +402,7 @@ def write_html( Figure object or dict representing a figure file: str or writeable A string representing a local file path or a writeable object - (e.g. an open file descriptor) + (e.g. a pathlib.Path object or an open file descriptor) config: dict or None (default None) Plotly.js figure config options auto_play: bool (default=True) @@ -520,24 +521,31 @@ def write_html( ) # Check if file is a string - file_is_str = isinstance(file, six.string_types) + if isinstance(file, six.string_types): + # Use the standard pathlib constructor to make a pathlib object. + path = Path(file) + elif isinstance(file, Path): # PurePath is the most general pathlib object. + # `file` is already a pathlib object. + path = file + else: + # We could not make a pathlib object out of file. Either `file` is an open file + # descriptor with a `write()` method or it's an invalid object. + path = None # Write HTML string - if file_is_str: - with open(file, "w") as f: - f.write(html_str) + if path is not None: + path.write_text(html_str) else: file.write(html_str) # Check if we should copy plotly.min.js to output directory - if file_is_str and full_html and include_plotlyjs == "directory": - bundle_path = os.path.join(os.path.dirname(file), "plotly.min.js") + if path is not None and full_html and include_plotlyjs == "directory": + bundle_path = path.parent / "plotly.min.js" - if not os.path.exists(bundle_path): - with open(bundle_path, "w") as f: - f.write(get_plotlyjs()) + if not bundle_path.exists(): + bundle_path.write_text(get_plotlyjs()) # Handle auto_open - if file_is_str and full_html and auto_open: - url = "file://" + os.path.abspath(file) + if path is not None and full_html and auto_open: + url = path.as_uri() webbrowser.open(url) diff --git a/packages/python/plotly/plotly/io/_kaleido.py b/packages/python/plotly/plotly/io/_kaleido.py index dcbe7f8ea72..d7264bd06f6 100644 --- a/packages/python/plotly/plotly/io/_kaleido.py +++ b/packages/python/plotly/plotly/io/_kaleido.py @@ -2,7 +2,7 @@ from six import string_types import os import json -from pathlib import Path, PurePath +from pathlib import Path import plotly from plotly.io._utils import validate_coerce_fig_to_dict @@ -232,13 +232,13 @@ def write_image( # Try to cast `file` as a pathlib object `path`. # ---------------------------------------------- if isinstance(file, string_types): - # Use the standard pathlib constructor to make a pathlib object. + # Use the standard Path constructor to make a pathlib object. path = Path(file) - elif isinstance(file, PurePath): # PurePath is the most general pathlib object. - # `file` is already a pathlib object. + elif isinstance(file, Path): + # `file` is already a Path object. path = file else: - # We could not make a pathlib object out of file. Either `file` is an open file + # We could not make a Path object out of file. Either `file` is an open file # descriptor with a `write()` method or it's an invalid object. path = None diff --git a/packages/python/plotly/plotly/tests/test_io/test_pathlib.py b/packages/python/plotly/plotly/tests/test_io/test_pathlib.py new file mode 100644 index 00000000000..48eab2bb9e5 --- /dev/null +++ b/packages/python/plotly/plotly/tests/test_io/test_pathlib.py @@ -0,0 +1,57 @@ +"""Test compatibility with pathlib.Path. + +See also relevant tests in + packages/python/plotly/plotly/tests/test_optional/test_kaleido/test_kaleido.py +""" + +from unittest import mock +import plotly.io as pio +from io import StringIO +from pathlib import Path +import re +from unittest.mock import Mock + +fig = {"layout": {"title": {"text": "figure title"}}} + + +def test_write_html(): + """Verify that various methods for producing HTML have equivalent results. + + The results will not be identical because the div id is pseudorandom. Thus + we compare the results after replacing the div id. + + We test the results of + - pio.to_html + - pio.write_html with a StringIO buffer + - pio.write_html with a mock pathlib Path + - pio.write_html with a mock file descriptor + """ + # Test pio.to_html + html = pio.to_html(fig) + + # Test pio.write_html with a StringIO buffer + sio = StringIO() + pio.write_html(fig, sio) + sio.seek(0) # Rewind to the beginning of the buffer, otherwise read() returns ''. + sio_html = sio.read() + assert replace_div_id(html) == replace_div_id(sio_html) + + # Test pio.write_html with a mock pathlib Path + mock_pathlib_path = Mock(spec=Path) + pio.write_html(fig, mock_pathlib_path) + mock_pathlib_path.write_text.assert_called_once() + (pl_html,) = mock_pathlib_path.write_text.call_args[0] + assert replace_div_id(html) == replace_div_id(pl_html) + + # Test pio.write_html with a mock file descriptor + mock_file_descriptor = Mock() + del mock_file_descriptor.write_bytes + pio.write_html(fig, mock_file_descriptor) + mock_file_descriptor.write.assert_called_once() + (fd_html,) = mock_file_descriptor.write.call_args[0] + assert replace_div_id(html) == replace_div_id(fd_html) + + +def replace_div_id(s): + uuid = re.search(r'