Refactor can-prefixed properties

docs/adapter-api.rst

@@ -52,29 +52,29 @@ functions and properties first:
 * ``__init__``: Used to initialize your adapter. See the
   :class:`sublime.adapters.Adapter.__init__` documentation for the function
   signature of the ``__init__`` function.
-* ``can_service_requests``: This property which will tell the UI whether or not
+* ``ping_status``: Assuming that your adapter requires connection to the
-  your adapter can currently service requests. (See the
+  internet, this property needs to be implemented. (If your adapter doesn't
-  :class:`sublime.adapters.Adapter.can_service_requests` documentation for
+  require connection to the internet, set
-  examples of what you may want to check in this property.)
+  :class:`sublime.adapters.Adapter.is_networked` to ``False`` and ignore the
+  rest of this bullet point.)
+
+  This property will tell the UI whether or not the underlying server can be
+  pinged.
 
   .. warning::
 
      This function is called *a lot* (probably too much?) so it *must* return a
-     value *instantly*. **Do not** perform a network request in this function.
+     value *instantly*. **Do not** perform the actual network request in this
-     If your adapter depends on connection to the network use a periodic ping
+     function. Instead, use a periodic ping that updates a state variable that
-     that updates a state variable that this function returns.
+     this function returns.
+
+.. TODO: these are totally wrong
 
 * ``get_config_parameters``: Specifies the settings which can be configured on
   for the adapter. See :ref:`adapter-api:Handling Configuration` for details.
 * ``verify_configuration``: Verifies whether or not a given set of configuration
   values are valid. See :ref:`adapter-api:Handling Configuration` for details.
 
-.. tip::
-
-   While developing the adapter, setting ``can_service_requests`` to ``True``
-   will indicate to the UI that your adapter is always ready to service
-   requests. This can be a useful debugging tool.
-
 .. note::
 
    The :class:`sublime.adapters.Adapter` class is an `Abstract Base Class
@@ -118,21 +118,12 @@ to implement the actual adapter data retrieval functions.
 
 For each data retrieval function there is a corresponding ``can_``-prefixed
 property (CPP) which will be used by the UI to determine if the data retrieval
-function can be called at the given time. If the CPP is ``False``, the UI will
+function can be called. If the CPP is ``False``, the UI will never call the
-never call the corresponding function (and if it does, it's a UI bug). The CPP
+corresponding function (and if it does, it's a UI bug). The CPP can be dynamic,
-can be dynamic, for example, if your adapter supports many API versions, some of
+for example, if your adapter supports many API versions, some of the CPPs may
-the CPPs may depend on the API version.
+depend on the API version. However, CPPs should not be dependent on connection
-
+status (there are times where the user may want to force a connection retry,
-There is a special, global ``can_``-prefixed property which determines whether
+even if the most recent ping failed).
-the adapter can currently service *any* requests. This should be used for checks
-such as making sure that the user is able to access the server. (However, this
-must be done in a non-blocking manner since this is called *a lot*.)
-
-.. code:: python
-
-    @property
-    def can_service_requests(self) -> bool:
-        return self.cached_ping_result_is_ok()
 
 Here is an example of what a ``get_playlists`` interface for an external server
 might look:
@@ -155,7 +146,7 @@ might look:
    ``True``.*
 
    \* At the moment, this isn't really the case and the UI just kinda explodes
-   if it doesn't have some of the functions available, but in the future guards
+   if it doesn't have some of the functions available, but in the future, guards
    will be added around all of the function calls.
 
 Usage Parameters

sublime/adapters/adapter_base.py

@@ -286,6 +286,10 @@ class Adapter(abc.ABC):
         """
         return True
 
+    # Network Properties
+    # These properties determine whether or not the adapter requires connection over a
+    # network and whether the underlying server can be pinged.
+    # ==================================================================================
     @property
     def is_networked(self) -> bool:
         """
@@ -299,68 +303,56 @@ class Adapter(abc.ABC):
     @abc.abstractmethod
     def ping_status(self) -> bool:
         """
-        This function should return whether or not the server can be pinged, however it
+        If the adapter :class:`is_networked`, then this function should return whether
-        must do it *instantly*. This function is called *very* often, and even a few
+        or not the server can be pinged. This function must provide an answer
-        milliseconds delay stacks up quickly and can block the UI thread.
+        *instantly* (it can't actually ping the server). This function is called *very*
+        often, and even a few milliseconds delay stacks up quickly and can block the UI
+        thread.
 
         One option is to ping the server every few seconds and cache the result of the
         ping and use that as the result of this function.
         """
 
     # Availability Properties
-    # These properties determine if what things the adapter can be used to do
+    # These properties determine if what things the adapter can be used to do. These
-    # at the current moment.
+    # properties can be dynamic based on things such as underlying API version, or other
+    # factors like that. However, these properties should not be dependent on the
+    # connection state. After the initial sync, these properties are expected to be
+    # constant.
     # ==================================================================================
-    # TODO: change these to just be "if it has the attribute, then you can try the
-    # action" and use "ping_status" to determine online/offline status.
-    @property
-    @abc.abstractmethod
-    def can_service_requests(self) -> bool:
-        """
-        Specifies whether or not the adapter can currently service requests. If this is
-        ``False``, none of the other data retrieval functions are expected to work.
-
-        This property must be server *instantly*. This function is called *very* often,
-        and even a few milliseconds delay stacks up quickly and can block the UI thread.
-
-        For example, if your adapter requires access to an external service, on option
-        is to ping the server every few seconds and cache the result of the ping and use
-        that as the return value of this function.
-        """
-
     # Playlists
     @property
     def can_get_playlists(self) -> bool:
         """
-        Whether the adapter supports :class:`get_playlist`.
+        Whether or not the adapter supports :class:`get_playlist`.
         """
         return False
 
     @property
     def can_get_playlist_details(self) -> bool:
         """
-        Whether :class:`get_playlist_details` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`get_playlist_details`.
         """
         return False
 
     @property
     def can_create_playlist(self) -> bool:
         """
-        Whether :class:`create_playlist` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`create_playlist`.
         """
         return False
 
     @property
     def can_update_playlist(self) -> bool:
         """
-        Whether :class:`update_playlist` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`update_playlist`.
         """
         return False
 
     @property
     def can_delete_playlist(self) -> bool:
         """
-        Whether :class:`delete_playlist` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`delete_playlist`.
         """
         return False
 
@@ -380,20 +372,20 @@ class Adapter(abc.ABC):
     @property
     def can_get_cover_art_uri(self) -> bool:
         """
-        Whether :class:`get_cover_art_uri` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`get_cover_art_uri`.
         """
 
     @property
     def can_stream(self) -> bool:
         """
-        Whether or not the adapter can provide a stream URI right now.
+        Whether or not the adapter can provide a stream URI.
         """
         return False
 
     @property
     def can_get_song_uri(self) -> bool:
         """
-        Whether :class:`get_song_uri` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`get_song_uri`.
         """
         return False
 
@@ -401,14 +393,14 @@ class Adapter(abc.ABC):
     @property
     def can_get_song_details(self) -> bool:
         """
-        Whether :class:`get_song_details` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`get_song_details`.
         """
         return False
 
     @property
     def can_scrobble_song(self) -> bool:
         """
-        Whether :class:`scrobble_song` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`scrobble_song`.
         """
         return False
 
@@ -426,21 +418,21 @@ class Adapter(abc.ABC):
     @property
     def can_get_artists(self) -> bool:
         """
-        Whether :class:`get_aritsts` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`get_aritsts`.
         """
         return False
 
     @property
     def can_get_artist(self) -> bool:
         """
-        Whether :class:`get_aritst` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`get_aritst`.
         """
         return False
 
     @property
     def can_get_ignored_articles(self) -> bool:
         """
-        Whether :class:`get_ignored_articles` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`get_ignored_articles`.
         """
         return False
 
@@ -448,14 +440,14 @@ class Adapter(abc.ABC):
     @property
     def can_get_albums(self) -> bool:
         """
-        Whether :class:`get_albums` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`get_albums`.
         """
         return False
 
     @property
     def can_get_album(self) -> bool:
         """
-        Whether :class:`get_album` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`get_album`.
         """
         return False
 
@@ -463,7 +455,7 @@ class Adapter(abc.ABC):
     @property
     def can_get_directory(self) -> bool:
         """
-        Whether :class:`get_directory` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`get_directory`.
         """
         return False
 
@@ -471,7 +463,7 @@ class Adapter(abc.ABC):
     @property
     def can_get_genres(self) -> bool:
         """
-        Whether :class:`get_genres` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`get_genres`.
         """
         return False
 
@@ -479,14 +471,14 @@ class Adapter(abc.ABC):
     @property
     def can_get_play_queue(self) -> bool:
         """
-        Whether :class:`get_play_queue` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`get_play_queue`.
         """
         return False
 
     @property
     def can_save_play_queue(self) -> bool:
         """
-        Whether :class:`save_play_queue` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`save_play_queue`.
         """
         return False
 
@@ -494,7 +486,7 @@ class Adapter(abc.ABC):
     @property
     def can_search(self) -> bool:
         """
-        Whether :class:`search` can be called on the adapter right now.
+        Whether or not the adapter supports :class:`search`.
         """
         return False
 

sublime/adapters/filesystem/adapter.py

@@ -79,7 +79,6 @@ class FilesystemAdapter(CachingAdapter):
     # ==================================================================================
     can_be_cached = False  # Can't be cached (there's no need).
     is_networked = False  # Doesn't access the network.
-    can_service_requests = True  # Can always be used to service requests.
 
     # TODO (#200) make these dependent on cache state. Need to do this kinda efficiently
     can_get_cover_art_uri = True

sublime/adapters/manager.py

@@ -275,18 +275,8 @@ class AdapterManager:
     TAdapter = TypeVar("TAdapter", bound=Adapter)
 
     @staticmethod
-    def _adapter_can_do(adapter: Optional[TAdapter], action_name: str) -> bool:
+    def _adapter_can_do(adapter: TAdapter, action_name: str) -> bool:
-        return (
+        return adapter is not None and getattr(adapter, f"can_{action_name}", False)
-            adapter is not None
-            and adapter.can_service_requests
-            and getattr(adapter, f"can_{action_name}", False)
-        )
-
-    @staticmethod
-    def _cache_can_do(action_name: str) -> bool:
-        return AdapterManager._instance is not None and AdapterManager._adapter_can_do(
-            AdapterManager._instance.caching_adapter, action_name
-        )
 
     @staticmethod
     def _ground_truth_can_do(action_name: str) -> bool:
@@ -302,16 +292,13 @@ class AdapterManager:
     def _can_use_cache(force: bool, action_name: str) -> bool:
         if force:
             return False
-        return AdapterManager._cache_can_do(action_name)
+        return (
-
+            AdapterManager._instance is not None
-    @staticmethod
+            and AdapterManager._instance.caching_adapter is not None
-    def _any_adapter_can_do(action_name: str) -> bool:
+            and AdapterManager._adapter_can_do(
-        if AdapterManager._instance is None:
+                AdapterManager._instance.caching_adapter, action_name
-            return False
+            )
-
+        )
-        return AdapterManager._ground_truth_can_do(
-            action_name
-        ) or AdapterManager._cache_can_do(action_name)
 
     @staticmethod
     def _create_ground_truth_result(
@@ -557,27 +544,27 @@ class AdapterManager:
     # ==================================================================================
     @staticmethod
     def can_get_playlists() -> bool:
-        return AdapterManager._any_adapter_can_do("get_playlists")
+        return AdapterManager._ground_truth_can_do("get_playlists")
 
     @staticmethod
     def can_get_playlist_details() -> bool:
-        return AdapterManager._any_adapter_can_do("get_playlist_details")
+        return AdapterManager._ground_truth_can_do("get_playlist_details")
 
     @staticmethod
     def can_create_playlist() -> bool:
-        return AdapterManager._any_adapter_can_do("create_playlist")
+        return AdapterManager._ground_truth_can_do("create_playlist")
 
     @staticmethod
     def can_update_playlist() -> bool:
-        return AdapterManager._any_adapter_can_do("update_playlist")
+        return AdapterManager._ground_truth_can_do("update_playlist")
 
     @staticmethod
     def can_delete_playlist() -> bool:
-        return AdapterManager._any_adapter_can_do("delete_playlist")
+        return AdapterManager._ground_truth_can_do("delete_playlist")
 
     @staticmethod
     def can_get_song_filename_or_stream() -> bool:
-        return AdapterManager._any_adapter_can_do("get_song_uri")
+        return AdapterManager._ground_truth_can_do("get_song_uri")
 
     @staticmethod
     def can_batch_download_songs() -> bool:
@@ -586,23 +573,23 @@ class AdapterManager:
 
     @staticmethod
     def can_get_genres() -> bool:
-        return AdapterManager._any_adapter_can_do("get_genres")
+        return AdapterManager._ground_truth_can_do("get_genres")
 
     @staticmethod
     def can_scrobble_song() -> bool:
-        return AdapterManager._any_adapter_can_do("scrobble_song")
+        return AdapterManager._ground_truth_can_do("scrobble_song")
 
     @staticmethod
     def can_get_artists() -> bool:
-        return AdapterManager._any_adapter_can_do("get_artists")
+        return AdapterManager._ground_truth_can_do("get_artists")
 
     @staticmethod
     def can_get_artist() -> bool:
-        return AdapterManager._any_adapter_can_do("get_artist")
+        return AdapterManager._ground_truth_can_do("get_artist")
 
     @staticmethod
     def can_get_directory() -> bool:
-        return AdapterManager._any_adapter_can_do("get_directory")
+        return AdapterManager._ground_truth_can_do("get_directory")
 
     @staticmethod
     def can_get_play_queue() -> bool:
@@ -614,7 +601,7 @@ class AdapterManager:
 
     @staticmethod
     def can_search() -> bool:
-        return AdapterManager._any_adapter_can_do("search")
+        return AdapterManager._ground_truth_can_do("search")
 
     # Data Retrieval Methods
     # ==================================================================================
@@ -1017,7 +1004,7 @@ class AdapterManager:
     @staticmethod
     def _get_ignored_articles(use_ground_truth_adapter: bool) -> Set[str]:
         # TODO (#21) get this at first startup.
-        if not AdapterManager._any_adapter_can_do("get_ignored_articles"):
+        if not AdapterManager._ground_truth_can_do("get_ignored_articles"):
             return set()
         try:
             ignored_articles: Set[str] = AdapterManager._get_from_cache_or_ground_truth(

sublime/adapters/subsonic/adapter.py

@@ -158,6 +158,7 @@ class SubsonicAdapter(Adapter):
     def _set_ping_status(self):
         now = datetime.now().timestamp()
         if now - self._last_ping_timestamp.value < 15:
+            print("ohea")
             return
 
         try:
@@ -172,10 +173,6 @@ class SubsonicAdapter(Adapter):
     def ping_status(self) -> bool:
         return self._server_available.value
 
-    @property
-    def can_service_requests(self) -> bool:
-        return self._server_available.value
-
     # TODO (#199) make these way smarter
     can_get_playlists = True
     can_get_playlist_details = True
@@ -271,8 +268,8 @@ class SubsonicAdapter(Adapter):
 
         if self._is_mock:
             logging.info("Using mock data")
-            return self._get_mock_data()
+            result = self._get_mock_data()
-
+        else:
             result = requests.get(
                 url, params=params, verify=not self.disable_cert_verify, timeout=timeout
             )
@@ -324,6 +321,8 @@ class SubsonicAdapter(Adapter):
 
     def _set_mock_data(self, data: Any):
         class MockResult:
+            status_code = 200
+
             def __init__(self, content: Any):
                 self._content = content
 

tests/adapter_tests/subsonic_adapter_tests.py

@@ -3,6 +3,7 @@ import logging
 import re
 from datetime import datetime, timedelta, timezone
 from pathlib import Path
+from time import sleep
 from typing import Any, Generator, List, Tuple
 
 import pytest
@@ -87,22 +88,23 @@ def test_request_making_methods(adapter: SubsonicAdapter):
     assert adapter._make_url("foo") == "http://subsonic.example.com/rest/foo.view"
 
 
-def test_can_service_requests(adapter: SubsonicAdapter):
+def test_ping_status(adapter: SubsonicAdapter):
     # Mock a connection error
     adapter._set_mock_data(Exception())
-    assert not adapter.can_service_requests
+    assert not adapter.ping_status
 
     # Simulate some sort of ping error
     for filename, data in mock_data_files("ping_failed"):
         logging.info(filename)
         logging.debug(data)
         adapter._set_mock_data(data)
-        assert not adapter.can_service_requests
+        assert not adapter.ping_status
 
     # Simulate valid ping
     adapter._set_mock_data(mock_json())
+    adapter._last_ping_timestamp.value = 0.0
     adapter._set_ping_status()
-    assert adapter.can_service_requests
+    assert adapter.ping_status
 
 
 def test_get_playlists(adapter: SubsonicAdapter):