Repository: intake/intake
Pull request #502 (2020-06-03 22:20:27)

MNT: clean up remaining leakage of CatalogEntry to public API
We found another place Entry objects were leaking out to the user via `cat.items()`.  This PR makes `cat.items()` consistent with item and attribute access by returning `DataSource` objects and deals with the internal fallout.

Added a test.




Patch:
diff --git a/intake/catalog/base.py b/intake/catalog/base.py
index 2c46b10..187b9f4 100644
--- a/intake/catalog/base.py
+++ b/intake/catalog/base.py
@@ -202,12 +202,20 @@ class Catalog(DataSource):
         return cat
 
     def filter(self, func):
-        """Create a Catalog of a subset of entries based on a condition
+        """
+        Create a Catalog of a subset of entries based on a condition
+
+        .. warning ::
+
+           This function operates on CatalogEntry objects not DataSource
+           objects.
 
-        Note that, whatever specific class this is performed on, the return
-        instance is a Catalog. The entries are passed unmodified, so they
-        will still reference the original catalog instance and include its
-        details such as directory,.
+        .. note ::
+
+            Note that, whatever specific class this is performed on,
+            the return instance is a Catalog. The entries are passed
+            unmodified, so they will still reference the original catalog
+            instance and include its details such as directory,.
 
         Parameters
         ----------
@@ -218,9 +226,10 @@ class Catalog(DataSource):
 
         Returns
         -------
-        New Catalog
+        Catalog
+           New catalog with Entries that still refer to their parents
         """
-        return Catalog.from_dict({key: entry for key, entry in self.items()
+        return Catalog.from_dict({key: entry for key, entry in self._entries.items()
                                   if func(entry)})
 
     @reload_on_change
@@ -257,8 +266,14 @@ class Catalog(DataSource):
         return out
 
     def items(self):
-        """Get an iterator over (key, value) tuples for the catalog entries."""
-        return self._get_entries().items()
+        """Get an iterator over (key, source) tuples for the catalog entries."""
+        for name, entry in self._get_entries().items():
+            yield name, entry()
+
+    def values(self):
+        """Get an iterator over the sources for catalog entries."""
+        for entry in self._get_entries().values():
+            yield entry()
 
     def serialize(self):
         """
@@ -270,7 +285,7 @@ class Catalog(DataSource):
         import yaml
         output = {"metadata": self.metadata, "sources": {},
                   "name": self.name}
-        for key, entry in self.items():
+        for key, entry in self._entries.items():
             kw = entry._captured_init_kwargs.copy()
             kw.pop('catalog', None)
             kw['parameters'] = {k.name: k.__getstate__()['kwargs'] for k in kw.get('parameters', [])}
@@ -304,9 +319,13 @@ class Catalog(DataSource):
         return self._entries
 
     def __iter__(self):
-        """Return an iterator over catalog entries."""
+        """Return an iterator over catalog entry names."""
         return iter(self._get_entries())
 
+    def keys(self):
+        """Entry names in this catalog as an iterator (alias for __iter__)"""
+        return iter(self)
+
     def __len__(self):
         return len(self._get_entries())
 
@@ -431,7 +450,7 @@ class Entries(collections.abc.Mapping):
     """Fetches entries from server on item lookup and iteration.
 
     This fetches pages of entries from the server during iteration and
-    caches them. On __getitem__ it fetches the sepcific entry from the
+    caches them. On __getitem__ it fetches the specific entry from the
     server.
     """
     # This has PY3-style lazy methods (keys, values, items). Since it's
@@ -772,7 +791,9 @@ class RemoteCatalog(Catalog):
         if not isinstance(cat, Catalog):
             raise NotImplementedError
         out = {}
-        for name, entry in cat.items():
+        # reach down into the private state because we apparently need the
+        # Entry here rather than the public facing DataSource objects.
+        for name, entry in cat._entries.items():
             out[name] = entry.__getstate__()
             out[name]['parameters'] = [up._captured_init_kwargs for up
                                        in entry._user_parameters]
diff --git a/intake/gui/__init__.py b/intake/gui/__init__.py
index 600f1b8..777b4bc 100644
--- a/intake/gui/__init__.py
+++ b/intake/gui/__init__.py
@@ -4,11 +4,26 @@
 #
 # The full license is in the LICENSE file, distributed with this software.
 #-----------------------------------------------------------------------------
+from distutils.version import LooseVersion
 
 
 def do_import():
+    error = too_old = False
     try:
         import panel as pn
+        too_old = LooseVersion(pn.__version__) < LooseVersion("0.9.5")
+    except ImportError as e:
+        error = e
+
+    if too_old or error:
+        class GUI(object):
+            def __repr__(self):
+                raise RuntimeError("Please install panel to use the GUI `conda "
+                                   "install -c conda-forge panel>=0.8.0`. Import "
+                                   "failed with error: %s" % error)
+        return GUI
+
+    try:
         from .gui import GUI
         css = """
         .scrolling {
@@ -18,15 +33,6 @@ def do_import():
         pn.config.raw_css.append(css)  # add scrolling class from css (panel GH#383, GH#384)
         pn.extension()
 
-    except ImportError as e:
-        error = e
-        
-        class GUI(object):
-            def __repr__(self):
-                raise RuntimeError("Please install panel to use the GUI `conda "
-                                   "install -c conda-forge panel>=0.8.0`. Import "
-                                   "failed with error: %s" % error)
-
     except Exception as e:
 
         class GUI(object):
@@ -34,7 +40,7 @@ def do_import():
                 raise RuntimeError("Initialisation of GUI failed, even though "
                                    "panel is installed. Please update it "
                                    "to a more recent version (`conda install -c"
-                                   " conda-forge panel>=0.7.0`).")
+                                   " conda-forge panel>=0.9.5`).")
     return GUI
 
 
diff --git a/intake/gui/base.py b/intake/gui/base.py
index dbcf2b0..bc3cc66 100644
--- a/intake/gui/base.py
+++ b/intake/gui/base.py
@@ -17,6 +17,7 @@ ICONS = {
     'error': os.path.join(here, 'icons', 'baseline-error-24px.svg'),
 }
 
+
 def enable_widget(widget, enable=True):
     """Set disabled on widget"""
     widget.disabled = not enable
diff --git a/intake/gui/catalog/select.py b/intake/gui/catalog/select.py
index f4d26ad..86126d4 100644
--- a/intake/gui/catalog/select.py
+++ b/intake/gui/catalog/select.py
@@ -93,7 +93,7 @@ class CatSelector(BaseSelector):
         right = '└──'
 
         def get_children(parent):
-            return [e() for k, e in parent.items()
+            return [e() for k, e in parent._entries.items()
                     if e.describe()['container'] == 'catalog']
 
         if len(cats) == 0:
diff --git a/intake/gui/source/select.py b/intake/gui/source/select.py
index 6e86c5d..22db2e8 100644
--- a/intake/gui/source/select.py
+++ b/intake/gui/source/select.py
@@ -89,7 +89,7 @@ class SourceSelector(BaseSelector):
         """Set sources from a list of cats"""
         sources = []
         for cat in coerce_to_list(cats):
-            sources.extend([entry for k, entry in cat.items()
+            sources.extend([entry for k, entry in cat._entries.items()
                             if entry.describe()['container'] != 'catalog'])
         self.items = sources
 


Test patch:
diff --git a/intake/catalog/tests/test_local.py b/intake/catalog/tests/test_local.py
index 7403b43..d271afc 100644
--- a/intake/catalog/tests/test_local.py
+++ b/intake/catalog/tests/test_local.py
@@ -722,3 +722,32 @@ def test_cat_add(tmpdir):
     # was added to the file
     cat = open_catalog(fn)
     assert list(cat) == ['cat']
+
+
+def test_no_entries_items(catalog1):
+    from intake.catalog.entry import CatalogEntry
+    from intake.source.base import DataSource
+
+    for k, v in catalog1.items():
+        assert not isinstance(v, CatalogEntry)
+        assert isinstance(v, DataSource)
+
+    for k in catalog1:
+        v = catalog1[k]
+        assert not isinstance(v, CatalogEntry)
+        assert isinstance(v, DataSource)
+
+    for k in catalog1:
+        # we can't do attribute access on "text" because it
+        # collides with a property
+        if k == 'text':
+            continue
+        v = getattr(catalog1, k)
+        assert not isinstance(v, CatalogEntry)
+        assert isinstance(v, DataSource)
+
+
+def test_cat_dictlike(catalog1):
+    assert list(catalog1) == list(catalog1.keys())
+    assert len(list(catalog1)) == len(catalog1)
+    assert list(catalog1.items()) == list(zip(catalog1.keys(), catalog1.values()))
diff --git a/intake/gui/source/tests/test_gui.py b/intake/gui/source/tests/test_gui.py
index 593e782..e1bdd86 100644
--- a/intake/gui/source/tests/test_gui.py
+++ b/intake/gui/source/tests/test_gui.py
@@ -4,10 +4,13 @@
 #
 # The full license is in the LICENSE file, distributed with this software.
 #-----------------------------------------------------------------------------
+from distutils.version import LooseVersion
 import pytest
 pn = pytest.importorskip('panel')
+too_old = LooseVersion(pn.__version__) < LooseVersion("0.9.5")
 
 
+@pytest.mark.skipif(too_old, reason="Use with latest panel")
 @pytest.fixture
 def gui(sources1):
     from ..gui import SourceGUI