Clean up TODOs

Author: schloerke

shiny/bookmark/_bookmark.py

@@ -83,7 +83,6 @@
 
 class Bookmark(ABC):
 
-    # TODO: Barret - This feels like it needs to be a weakref
     _session_root: Session
     """
     The root session object (most likely a `AppSession` object).
@@ -145,7 +144,6 @@ def __init__(self, session_root: Session):
         # from ._restore_state import RestoreContext
 
         super().__init__()
-        # TODO: Barret - Q: Should this be a weakref; Session -> Bookmark -> Session
         self._session_root = session_root
         self._restore_context_value = None
         self._store = MISSING
@@ -248,7 +246,6 @@ async def update_query_string(
         ...
 
     @abstractmethod
-    # TODO: Barret - Q: Rename to `update()`? `session.bookmark.update()`?
     async def do_bookmark(self) -> None:
         """
         Perform bookmarking.
@@ -521,7 +518,6 @@ def __init__(self, session_proxy: SessionProxy):
         super().__init__(session_proxy.root_scope())
 
         self._ns = session_proxy.ns
-        # TODO: Barret - Q: Should this be a weakref
         self._session_proxy = session_proxy
 
         self._session_root.bookmark._proxy_exclude_fns.append(
@@ -625,11 +621,8 @@ def on_bookmarked(
         self,
         callback: Callable[[str], None] | Callable[[str], Awaitable[None]],
         /,
-    ) -> NoReturn:
-        # TODO: Barret - Q: Shouldn't we implement this? `self._root_bookmark.on_bookmark()`
-        raise NotImplementedError(
-            "Please call `.on_bookmarked()` from the root session only, e.g. `session.root_scope().bookmark.on_bookmark()`."
-        )
+    ) -> CancelCallback:
+        return self._on_bookmarked_callbacks.register(wrap_async(callback))
 
     def _get_bookmark_exclude(self) -> NoReturn:
         raise NotImplementedError(

shiny/bookmark/_external.py

@@ -37,6 +37,8 @@
     bound=Callable[[str], Awaitable[Path]] | Callable[[str], Awaitable[Path]],
 )
 
+# TODO: Barret - Integrate Set / Restore for Connect. Ex: Connect https://github.com/posit-dev/connect/blob/8de330aec6a61cf21e160b5081d08a1d3d7e8129/R/connect.R#L915
+
 
 def set_save_dir(fn: GetBookmarkDirT) -> GetBookmarkDirT:
     """TODO: Barret document"""

shiny/bookmark/_restore_state.py

@@ -59,7 +59,7 @@ def _state_within_namespace(self, prefix: str) -> "RestoreState":
             if self._name_has_namespace(name, prefix)
         }
 
-        # TODO: Barret; Is this for bookmarking?!?
+        # TODO: Barret; Is this for file inputs?!?
         dir = self.dir
         if dir is not None:
             dir = dir / prefix
@@ -194,7 +194,6 @@ async def _load_state_qs(self, query_string: str) -> None:
 
         if load_bookmark_fn is None:
             if is_hosted():
-                # TODO: Barret; Implement Connect's `bookmark_load_dir` function
                 raise NotImplementedError(
                     "The hosting environment does not support server-side bookmarking."
                 )

shiny/bookmark/_save_state.py

@@ -13,10 +13,6 @@
 from ._types import GetBookmarkSaveDir
 from ._utils import is_hosted, to_json_str
 
-# TODO: Barret - Set / Load SaveState for Connect. Ex: Connect https://github.com/posit-dev/connect/blob/8de330aec6a61cf21e160b5081d08a1d3d7e8129/R/connect.R#L915
-# Might need to have independent save/load functions to register to avoid a class constructor
-
-
 if TYPE_CHECKING:
     from .. import Inputs
 else: