fix(obslist): post-merge cleanup from PR #394 review (issues #2, #4)

python/PiFinder/obslist.py

@@ -292,35 +292,79 @@ def read_list(catalogs: Catalogs, name):
 
     list_catalog: list = []
     name_index = None  # built lazily, only if an entry needs name resolution
-    for i, entry in enumerate(obs_list.entries):
-        _object = None
-
-        # Try catalog resolution with catalog_names (skylist multi-name support)
-        if entry.catalog_names:
-            _object = resolve_object(entry.catalog_names, catalogs)
-        elif entry.catalog_code and entry.sequence:
-            _object = resolve_object(
-                [f"{entry.catalog_code} {entry.sequence}"], catalogs
-            )
-
-        # No catalog match: try resolving by object name (handles lists that
-        # identify objects only by name, e.g. carbon stars "VY Andromedae").
-        if not _object and entry.name:
-            if name_index is None:
-                name_index = _build_name_index(catalogs)
-            _object = resolve_by_name(entry.name, name_index)
-
-        # Resolved objects are shared catalog instances: record this list's
-        # description under the list name instead of clobbering the catalog one.
-        if _object and entry.description:
-            _object.list_descriptions[obs_list.name] = entry.description
+    errors = 0
+    last_error: Exception | None = None
+    try:
+        for i, entry in enumerate(obs_list.entries):
+            try:
+                _object = None
+
+                # Try catalog resolution with catalog_names (skylist multi-name)
+                if entry.catalog_names:
+                    _object = resolve_object(entry.catalog_names, catalogs)
+                elif entry.catalog_code and entry.sequence:
+                    _object = resolve_object(
+                        [f"{entry.catalog_code} {entry.sequence}"], catalogs
+                    )
 
-        # Fall back to coordinate-based object
-        if not _object and (entry.ra or entry.dec):
-            _object = _coordinate_object(entry, i)
+                # No catalog match: try resolving by object name (handles lists
+                # that identify objects only by name, e.g. "VY Andromedae").
+                if not _object and entry.name:
+                    if name_index is None:
+                        name_index = _build_name_index(catalogs)
+                    _object = resolve_by_name(entry.name, name_index)
+
+                # Resolved objects are shared catalog instances: record this
+                # list's description under the list name rather than clobbering
+                # the catalog one.
+                if _object and entry.description:
+                    _object.list_descriptions[obs_list.name] = entry.description
+
+                # Fall back to coordinate-based object
+                if not _object and (entry.ra or entry.dec):
+                    _object = _coordinate_object(entry, i)
+
+                if _object:
+                    list_catalog.append(_object)
+            except Exception as e:
+                # One malformed entry (bad coords, a lookup that errors) must not
+                # sink the whole list: log it, skip it, keep resolving the rest.
+                errors += 1
+                last_error = e
+                logger.warning(
+                    "Skipping observing list entry %d (%r): %s",
+                    i,
+                    getattr(entry, "name", "?"),
+                    e,
+                )
+    except Exception as e:
+        # Safety net for a failure outside the per-entry guard (e.g. the entries
+        # iterable itself raises). Mirror the file-read error-dict contract so
+        # UIObsList.key_right shows its message instead of crashing the UI. Use
+        # len(list_catalog), not len(obs_list.entries): if entries was the thing
+        # that raised, touching it again here would re-raise and defeat the net.
+        logger.critical("Failed to resolve observing list %s: %s", name, e)
+        return {
+            "result": "error",
+            "objects_parsed": len(list_catalog),
+            "message": str(e),
+            "catalog_objects": list_catalog,
+        }
 
-        if _object:
-            list_catalog.append(_object)
+    # Every entry raised: this is systemic (e.g. the catalog DB is unavailable),
+    # not a few bad rows. Report it as an error rather than a silent "0 objects".
+    if obs_list.entries and errors == len(obs_list.entries):
+        logger.critical(
+            "Failed to resolve any entries in observing list %s: %s",
+            name,
+            last_error,
+        )
+        return {
+            "result": "error",
+            "objects_parsed": len(obs_list.entries),
+            "message": str(last_error),
+            "catalog_objects": list_catalog,
+        }
 
     return {
         "result": "success",

python/PiFinder/ui/object_details.py

@@ -35,7 +35,10 @@
 
 
 # Read-only handle to the catalog DB, opened once and shared across detail
-# views (only used when cross-catalog descriptions are enabled).
+# views. Used by _other_catalog_descriptions() to pull an object's listings in
+# its *other* catalogs (this always runs on the description view). Like the
+# per-instance ObservationsDatabase opened below, this read connection lives for
+# the life of the UI process and is closed when that process exits.
 _objects_db = None
 
 

python/tests/test_obslist_resolve.py

@@ -2,11 +2,19 @@
 Tests for name-based observing-list resolution: constellation-genitive
 normalization and exact name lookup. These let lists that identify objects
 only by name (e.g. carbon stars "VY Andromedae") match the catalog.
+
+Also covers read_list()'s error handling: a single bad entry is skipped and the
+rest still load, while a systemic resolution failure is returned as the error
+dict (never propagated -- UIObsList.key_right has no handler and would crash).
 """
 
+from types import SimpleNamespace
+
 import pytest
 
+from PiFinder import obslist
 from PiFinder.obslist import _normalize_designation, resolve_by_name
+from PiFinder.obslist_formats import ObsList, ObsListEntry
 
 
 @pytest.mark.unit
@@ -47,3 +55,85 @@ def test_no_match(self):
 
     def test_empty_name(self):
         assert resolve_by_name("", {"x": 1}) is None
+
+
+def _entry(name, code, seq, desc=""):
+    """Minimal catalog-resolvable entry (read_list reads catalog_code+sequence)."""
+    return ObsListEntry(
+        name=name,
+        ra=10.0,
+        dec=20.0,
+        obj_type="Gx",
+        catalog_code=code,
+        sequence=seq,
+        description=desc,
+    )
+
+
+@pytest.mark.unit
+class TestReadListErrorHandling:
+    """read_list's resolution loop must not propagate exceptions to the UI."""
+
+    def test_per_entry_failure_skips_and_continues(self, monkeypatch):
+        # First entry's lookup raises, second resolves -- the list still loads.
+        obs_list = ObsList(
+            name="Mixed",
+            entries=[_entry("NGC 224", "NGC", 224), _entry("M 42", "M", 42)],
+        )
+        monkeypatch.setattr(obslist, "formats_read_file", lambda path: obs_list)
+
+        resolved = SimpleNamespace(list_descriptions={})
+
+        def fake_resolve(catalog_numbers, catalogs):
+            if "NGC 224" in catalog_numbers:
+                raise RuntimeError("bad row")
+            return resolved
+
+        monkeypatch.setattr(obslist, "resolve_object", fake_resolve)
+
+        result = obslist.read_list(catalogs=None, name="Mixed.skylist")
+
+        assert result["result"] == "success"
+        assert result["objects_parsed"] == 2
+        # The bad entry is dropped (not turned into a coordinate object); only
+        # the one that resolved survives.
+        assert result["catalog_objects"] == [resolved]
+
+    def test_all_entries_fail_returns_error(self, monkeypatch):
+        # Every lookup raises (e.g. catalog DB unavailable): systemic, so report
+        # an error rather than a silent "0 objects" success.
+        obs_list = ObsList(
+            name="AllBad",
+            entries=[_entry("NGC 224", "NGC", 224), _entry("M 42", "M", 42)],
+        )
+        monkeypatch.setattr(obslist, "formats_read_file", lambda path: obs_list)
+
+        def fake_resolve(catalog_numbers, catalogs):
+            raise RuntimeError("catalog db gone")
+
+        monkeypatch.setattr(obslist, "resolve_object", fake_resolve)
+
+        result = obslist.read_list(catalogs=None, name="AllBad.skylist")
+
+        assert result["result"] == "error"
+        assert result["objects_parsed"] == 2
+        assert result["catalog_objects"] == []
+        assert "catalog db gone" in result["message"]
+
+    def test_catastrophic_iteration_failure_returns_error(self, monkeypatch):
+        # Failure outside the per-entry guard (the entries iterable itself
+        # raises) hits the outer safety net and returns the error dict.
+        class ExplodingList:
+            name = "Boom"
+
+            @property
+            def entries(self):
+                raise RuntimeError("entries unavailable")
+
+        monkeypatch.setattr(obslist, "formats_read_file", lambda path: ExplodingList())
+
+        result = obslist.read_list(catalogs=None, name="Boom.skylist")
+
+        assert result["result"] == "error"
+        assert result["catalog_objects"] == []
+        assert "entries unavailable" in result["message"]