bug(data frame): Reduce usage of CellPatchProcessed in favor of CellPatch (#1526)

Author: schloerke

CHANGELOG.md

@@ -47,6 +47,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * Fixed #1440: When a Shiny Express app with a `www/` subdirectory was deployed to shinyapps.io or a Connect server, it would not start correctly. (#1442)
 
+* The return type for the data frame patch function now returns a list of `render.CellPatch` objects (which support `htmltools.TagNode` for the `value` attribute). These values will be set inside the data frame's `.data_view()` result. This also means that `.cell_patches()` will be a list of `render.CellPatch` objects.  (#1526)
+
 ### Other changes
 
 ## [0.10.2] - 2024-05-28

shiny/playwright/controller/_controls.py

@@ -17,8 +17,8 @@
 
 # Import `shiny`'s typing extentions.
 # Since this is a private file, tell pyright to ignore the import
-from ..._typing_extensions import TypeGuard, assert_type
-from ...types import MISSING, MISSING_TYPE
+from ..._typing_extensions import TypeGuard
+from ...types import MISSING, MISSING_TYPE, ListOrTuple
 from .._types import (
     AttrValue,
     ListPatternOrStr,
@@ -817,7 +817,7 @@ def __init__(
 
     def set(
         self,
-        selected: str | list[str],
+        selected: str | ListOrTuple[str],
         *,
         timeout: Timeout = None,
     ) -> None:
@@ -1558,8 +1558,11 @@ def expect_locator_contains_values_in_list(
             The key. Defaults to `"value"`.
         """
         # Make sure the locator contains all of `arr`
-
-        assert_type(arr, typing.List[str])
+        if not isinstance(arr, list):
+            raise TypeError(f"`{arr_name}` must be a list")
+        for item in arr:
+            if not isinstance(item, str):
+                raise TypeError(f"`{arr_name}` must be a list of strings")
 
         # Make sure the locator has len(uniq_arr) input elements
         _MultipleDomItems.assert_arr_is_unique(arr, f"`{arr_name}` must be unique")
@@ -1814,7 +1817,7 @@ def __init__(
     def set(
         self,
         # TODO-future: Allow `selected` to be a single Pattern to perform matching against many items
-        selected: list[str],
+        selected: ListOrTuple[str],
         *,
         timeout: Timeout = None,
         **kwargs: object,
@@ -1830,7 +1833,11 @@ def set(
             The timeout for the action. Defaults to `None`.
         """
         # Having an arr of size 0 is allowed. Will uncheck everything
-        assert_type(selected, typing.List[str])
+        if not isinstance(selected, list):
+            raise TypeError("`selected` must be a list or tuple")
+        for item in selected:
+            if not isinstance(item, str):
+                raise TypeError("`selected` must be a list of strings")
 
         # Make sure the selected items exist
         # Similar to `self.expect_choices(choices = selected)`, but with
@@ -1999,7 +2006,9 @@ def set(
         timeout
             The timeout for the action. Defaults to `None`.
         """
-        assert_type(selected, str)
+        if not isinstance(selected, str):
+            raise TypeError("`selected` must be a string")
+
         # Only need to set.
         # The Browser will _unset_ the previously selected radio button
         self.loc_container.locator(
@@ -3938,8 +3947,10 @@ def expect_cell(
         timeout
             The maximum time to wait for the text to appear. Defaults to `None`.
         """
-        assert_type(row, int)
-        assert_type(col, int)
+        if not isinstance(row, int):
+            raise TypeError("`row` must be an integer")
+        if not isinstance(col, int):
+            raise TypeError("`col` must be an integer")
         playwright_expect(
             self.loc.locator(
                 f"xpath=./table/tbody/tr[{row}]/td[{col}] | ./table/tbody/tr[{row}]/th[{col}]"
@@ -3994,7 +4005,8 @@ def expect_column_text(
         timeout
             The maximum time to wait for the text to appear. Defaults to `None`.
         """
-        assert_type(col, int)
+        if not isinstance(col, int):
+            raise TypeError("`col` must be an integer")
         playwright_expect(
             self.loc.locator(f"xpath=./table/tbody/tr/td[{col}]")
         ).to_have_text(
@@ -5981,8 +5993,10 @@ def expect_cell(
         timeout
             The maximum time to wait for the expectation to pass. Defaults to `None`.
         """
-        assert_type(row, int)
-        assert_type(col, int)
+        if not isinstance(row, int):
+            raise TypeError("`row` must be an integer.")
+        if not isinstance(col, int):
+            raise TypeError("`col` must be an integer.")
         self._cell_scroll_if_needed(row=row, col=col, timeout=timeout)
         playwright_expect(self.cell_locator(row, col)).to_have_text(
             value, timeout=timeout
@@ -6078,7 +6092,8 @@ def _expect_column_label(
         timeout
             The maximum time to wait for the expectation to pass. Defaults to `None`.
         """
-        assert_type(col, int)
+        if not isinstance(col, int):
+            raise TypeError("`col` must be an integer.")
         # It's zero based, nth(0) selects the first element.
         playwright_expect(self.loc_column_label.nth(col - 1)).to_have_text(
             value,

shiny/render/_data_frame.py

@@ -28,8 +28,8 @@
     SelectionModes,
     as_cell_selection,
     assert_patches_shape,
-    wrap_shiny_html,
 )
+from ._data_frame_utils._html import maybe_as_cell_html
 from ._data_frame_utils._styles import as_browser_style_infos
 from ._data_frame_utils._tbl_data import (
     apply_frame_patches__typed,
@@ -195,7 +195,7 @@ class data_frame(Renderer[DataFrameResult[DataFrameLikeT]]):
     patches with updated values.
     """
 
-    _cell_patch_map: reactive.Value[dict[tuple[int, int], CellPatchProcessed]]
+    _cell_patch_map: reactive.Value[dict[tuple[int, int], CellPatch]]
     """
     Reactive dictionary of patches to be applied to the data frame.
 
@@ -204,7 +204,7 @@ class data_frame(Renderer[DataFrameResult[DataFrameLikeT]]):
 
     The key is defined as `(row_index, column_index)`.
     """
-    cell_patches: reactive.Calc_[list[CellPatchProcessed]]
+    cell_patches: reactive.Calc_[list[CellPatch]]
     """
     Reactive value of the data frame's edits provided by the user.
     """
@@ -349,7 +349,7 @@ def _init_reactives(self) -> None:
         self._cell_patch_map = reactive.Value({})
 
         @reactive.calc
-        def self_cell_patches() -> list[CellPatchProcessed]:
+        def self_cell_patches() -> list[CellPatch]:
             return list(self._cell_patch_map().values())
 
         self.cell_patches = self_cell_patches
@@ -537,8 +537,8 @@ async def patch_fn(
 
         async def patches_fn(
             *,
-            patches: list[CellPatch],
-        ):
+            patches: tuple[CellPatch, ...],
+        ) -> ListOrTuple[CellPatch]:
             ret_patches: list[CellPatch] = []
             for patch in patches:
 
@@ -583,7 +583,7 @@ def _set_patches_handler(self) -> str:
         return self._set_patches_handler_impl(self._patches_handler)
 
     # Do not change this method name unless you update corresponding code in `/js/dataframe/`!!
-    async def _patches_handler(self, patches: list[CellPatch]) -> Jsonifiable:
+    async def _patches_handler(self, patches: tuple[CellPatch, ...]) -> Jsonifiable:
         """
         Accepts edit patches requests from the client and returns the processed patches.
 
@@ -602,7 +602,8 @@ async def _patches_handler(self, patches: list[CellPatch]) -> Jsonifiable:
 
         with session_context(self._get_session()):
             # Call user's cell update method to retrieve formatted values
-            patches = await self._patches_fn(patches=patches)
+            val = await self._patches_fn(patches=patches)
+            patches = tuple(val)
 
         # Check to make sure `updated_infos` is a list of dicts with the correct keys
         bad_patches_format = not isinstance(patches, list)
@@ -625,29 +626,45 @@ async def _patches_handler(self, patches: list[CellPatch]) -> Jsonifiable:
                     )
 
         # Add (or overwrite) new cell patches by setting each patch into the cell patch map
-        processed_patches: list[Jsonifiable] = []
         for patch in patches:
-            processed_patch = self._set_cell_patch_map_value(
+            self._set_cell_patch_map_value(
                 value=patch["value"],
                 row_index=patch["row_index"],
                 column_index=patch["column_index"],
             )
-            processed_patches.append(
-                cell_patch_processed_to_jsonifiable(processed_patch)
-            )
+
+        # Upgrade any HTML-like content to `CellHtml` json objects
+        processed_patches: list[CellPatchProcessed] = [
+            {
+                "row_index": patch["row_index"],
+                "column_index": patch["column_index"],
+                # Only upgrade the value if it is necessary
+                "value": maybe_as_cell_html(
+                    patch["value"],
+                    session=self._get_session(),
+                ),
+            }
+            for patch in patches
+        ]
+
+        # Prep the processed patches for sending to the client
+        jsonifiable_patches: list[Jsonifiable] = [
+            cell_patch_processed_to_jsonifiable(ret_processed_patch)
+            for ret_processed_patch in processed_patches
+        ]
 
         await self._attempt_update_cell_style()
 
         # Return the processed patches to the client
-        return processed_patches
+        return jsonifiable_patches
 
     def _set_cell_patch_map_value(
         self,
         value: CellValue,
         *,
         row_index: int,
         column_index: int,
-    ) -> CellPatchProcessed:
+    ):
         """
         Set the value within the cell patch map.
 
@@ -671,18 +688,16 @@ def _set_cell_patch_map_value(
         # TODO-barret-render.data_frame; The `value` should be coerced by pandas to the correct type
         # TODO-barret; See https://pandas.pydata.org/pandas-docs/stable/user_guide/basics.html#object-conversion
 
-        cell_patch_processed: CellPatchProcessed = {
+        cell_patch: CellPatch = {
             "row_index": row_index,
             "column_index": column_index,
-            "value": wrap_shiny_html(value, session=self._get_session()),
+            "value": value,
         }
         # Use copy to set the new value
         cell_patch_map = self._cell_patch_map().copy()
-        cell_patch_map[(row_index, column_index)] = cell_patch_processed
+        cell_patch_map[(row_index, column_index)] = cell_patch
         self._cell_patch_map.set(cell_patch_map)
 
-        return cell_patch_processed
-
     async def _attempt_update_cell_style(self) -> None:
         with session_context(self._get_session()):
 
@@ -704,7 +719,7 @@ async def _attempt_update_cell_style(self) -> None:
     # TODO-barret-render.data_frame; Add `update_cell_value()` method
     # def _update_cell_value(
     #     self, value: CellValue, *, row_index: int, column_index: int
-    # ) -> CellPatchProcessed:
+    # ) -> CellPatch:
     #     """
     #     Update the value of a cell in the data frame.
     #

shiny/render/_data_frame_utils/__init__.py

@@ -3,7 +3,7 @@
     DataGrid,
     DataTable,
 )
-from ._html import wrap_shiny_html
+from ._html import maybe_as_cell_html
 from ._patch import (
     CellPatch,
     CellValue,
@@ -27,7 +27,7 @@
     "AbstractTabularData",
     "DataGrid",
     "DataTable",
-    "wrap_shiny_html",
+    "maybe_as_cell_html",
     "CellHtml",
     "CellPatch",
     "CellPatchProcessed",

shiny/render/_data_frame_utils/_html.py

@@ -12,17 +12,32 @@
     from ...session import Session
 
 
+def as_cell_html(x: TagNode, *, session: Session) -> CellHtml:
+    return {"isShinyHtml": True, "obj": session._process_ui(x)}
+
+
+# def is_cell_html(val: Any) -> TypeGuard[CellHtml]:
+#     return isinstance(val, dict) and (
+#         val.get("isShinyHtml", False)  # pyright: ignore[reportUnknownMemberType]
+#         is True
+#     )
+
+
+@overload
+def maybe_as_cell_html(  # pyright: ignore[reportOverlappingOverload]
+    x: TagNode, *, session: Session
+) -> CellHtml: ...
 @overload
-def wrap_shiny_html(  # pyright: ignore[reportOverlappingOverload]
+def maybe_as_cell_html(  # pyright: ignore[reportOverlappingOverload]
     x: TagNode, *, session: Session
 ) -> CellHtml: ...
 @overload
-def wrap_shiny_html(x: Jsonifiable, *, session: Session) -> Jsonifiable: ...
-def wrap_shiny_html(
+def maybe_as_cell_html(x: Jsonifiable, *, session: Session) -> Jsonifiable: ...
+def maybe_as_cell_html(
     x: Jsonifiable | TagNode, *, session: Session
 ) -> Jsonifiable | CellHtml:
     if is_shiny_html(x):
-        return {"isShinyHtml": True, "obj": session._process_ui(x)}
+        return as_cell_html(x, session=session)
     return cast(Jsonifiable, x)
 
 

shiny/render/_data_frame_utils/_pandas.py

@@ -6,7 +6,7 @@
 from htmltools import TagNode
 
 from ...session._utils import require_active_session
-from ._html import col_contains_shiny_html, wrap_shiny_html
+from ._html import col_contains_shiny_html, maybe_as_cell_html
 from ._tbl_data import PdDataFrame, frame_column_names
 from ._types import FrameDtype, FrameJson
 
@@ -55,7 +55,7 @@ def serialize_frame_pd(df: "pd.DataFrame") -> FrameJson:
             session = require_active_session(None)
 
             def wrap_shiny_html_with_session(x: TagNode):
-                return wrap_shiny_html(x, session=session)
+                return maybe_as_cell_html(x, session=session)
 
             for html_column in html_columns:
                 # _upgrade_ all the HTML columns to `CellHtml` json objects

shiny/render/_data_frame_utils/_patch.py

@@ -4,6 +4,7 @@
 # TODO-barret-render.data_frame; Add examples of patch!
 from typing import Protocol, Sequence
 
+from ...types import ListOrTuple
 from ._types import CellPatch, CellValue
 
 
@@ -19,8 +20,8 @@ class PatchesFn(Protocol):
     async def __call__(
         self,
         *,
-        patches: list[CellPatch],
-    ) -> list[CellPatch]: ...
+        patches: tuple[CellPatch, ...],
+    ) -> ListOrTuple[CellPatch]: ...
 
 
 class PatchFnSync(Protocol):
@@ -35,8 +36,8 @@ class PatchesFnSync(Protocol):
     def __call__(
         self,
         *,
-        patches: list[CellPatch],
-    ) -> list[CellPatch]: ...
+        patches: tuple[CellPatch, ...],
+    ) -> ListOrTuple[CellPatch]: ...
 
 
 def assert_patches_shape(x: Sequence[CellPatch]) -> None: