Apply suggestions from code review

Co-authored-by: Eero Vaher <eero.vaher@gmail.com>
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>

astropy/cosmology/io/mapping.py

@@ -20,8 +20,11 @@
 def _rename_map(map, /, renames):
     # So we are guaranteed to have a poppable map.
     # And apply renames, if given.
-    if not set(renames.values()).isdisjoint(map.keys()):
-        raise ValueError("'renames' values must be disjoint from 'map' keys.")
+    if common_names := set(renames.values()).intersection(map):
+        raise ValueError(
+            "'renames' values must be disjoint from 'map' keys, "
+            f"the common keys are: {common_names}"
+        )
     return {renames.get(k, k): v for k, v in map.items()}
 
 
@@ -34,9 +37,7 @@ def _get_cosmology_class(cosmology, params, /):
     else:
         params.pop("cosmology", None)  # pop, but don't use
     # if string, parse to class
-    if isinstance(cosmology, str):
-        cosmology = _COSMOLOGY_CLASSES[cosmology]
-    return cosmology
+    return _COSMOLOGY_CLASSES[cosmology] if isinstance(cosmology, str) else cosmology
 
 
 def from_mapping(map, *, move_to_meta=False, cosmology=None, rename=None):
@@ -106,7 +107,7 @@ class itself.
     """
     # Rename keys, if given a ``renames`` dict.
     # Also, make a copy of the mapping, so we can pop from it.
-    params = _rename_map(dict(map), renames={} if rename is None else rename)
+    params = _rename_map(dict(map), renames=rename or {})
 
     # Get cosmology class
     cosmology = _get_cosmology_class(cosmology, params)
@@ -145,7 +146,7 @@ def to_mapping(
 
     Parameters
     ----------
-    cosmology : |Cosmology|
+    cosmology : :class:`~astropy.cosmology.Cosmology`
         The cosmology instance to convert to a mapping.
     *args
         Not used. Needed for compatibility with
@@ -161,7 +162,8 @@ def to_mapping(
         `False`, default) or to merge with the rest of the mapping, preferring
         the original values (if `True`)
     rename : dict or None (optional, keyword-only)
-        A dictionary mapping fields of the Cosmology to keys in the map.
+        A `dict` mapping fields of the :class:`~astropy.cosmology.Cosmology` to keys in
+        the map.
 
     Returns
     -------
@@ -252,9 +254,7 @@ def to_mapping(
     if not move_from_meta:
         m["meta"] = meta  # TODO? should meta be type(cls)
     # Rename keys
-    if rename is not None:
-        m = _rename_map(m, rename)
-    return m
+    return m if rename is None else _rename_map(m, rename)
 
 
 def mapping_identify(origin, format, *args, **kwargs):

astropy/cosmology/io/row.py

@@ -63,12 +63,12 @@ def from_row(row, *, move_to_meta=False, cosmology=None, rename=None):
                       Tcmb0=2.7255 K, Neff=3.046, m_nu=[0. 0. 0.06] eV, Ob0=0.04897)
     """
     inv_rename = {v: k for k, v in rename.items()} if rename is not None else {}
-    kn = inv_rename.get("name", "name")
-    km = inv_rename.get("meta", "meta")
-    kc = inv_rename.get("cosmology", "cosmology")
+    kname = inv_rename.get("name", "name")
+    kmeta = inv_rename.get("meta", "meta")
+    kcosmo = inv_rename.get("cosmology", "cosmology")
 
     # special values
-    name = row[kn] if kn in row.columns else None  # get name from column
+    name = row[kname] if kname in row.columns else None  # get name from column
 
     meta = defaultdict(dict, copy.deepcopy(row.meta))
     # Now need to add the Columnar metadata. This is only available on the
@@ -80,9 +80,9 @@ def from_row(row, *, move_to_meta=False, cosmology=None, rename=None):
 
     # turn row into mapping, filling cosmo if not in a column
     mapping = dict(row)
-    mapping[kn] = name
-    mapping.setdefault(kc, meta.pop(kc, None))
-    mapping[km] = dict(meta)
+    mapping[kname] = name
+    mapping.setdefault(kcosmo, meta.pop(kcosmo, None))
+    mapping[kmeta] = dict(meta)
 
     # build cosmology from map
     return from_mapping(

astropy/cosmology/io/tests/test_mapping.py

@@ -129,29 +129,26 @@ def test_tofrom_mapping_move_tofrom_meta(self, cosmo, to_format, from_format):
         assert got == cosmo  # (Doesn't check metadata)
         assert got.meta["mismatching"] == "will error"
 
-    def test_to_mapping_rename_disjoint(self, cosmo, to_format):
-        """Test roundtrip of ``rename`` in ``to/from_mapping()``."""
-        # rename
+    def test_to_mapping_rename_conflict(self, cosmo, to_format):
+        """Test ``rename`` in ``to_mapping()``."""
         to_rename = {"name": "name"}
-        with pytest.raises(ValueError, match="disjoint"):
+        with pytest.raises(ValueError, match="'renames' values must be disjoint"):
             to_format("mapping", rename=to_rename)
 
     def test_from_mapping_rename_disjoint(self, cosmo, to_format, from_format):
-        """Test roundtrip of ``rename`` in ``to/from_mapping()``."""
-        # rename
+        """Test ``rename`` in `from_mapping()``."""
         m = to_format("mapping")
 
-        with pytest.raises(ValueError, match="disjoint"):
+        with pytest.raises(ValueError, match="'renames' values must be disjoint"):
             from_format(m, format="mapping", rename={"name": "name"})
 
-    def test_tofrom_mapping_rename(self, cosmo, to_format, from_format):
-        """Test roundtrip of ``rename`` in ``to/from_mapping()``."""
-        # rename
+    def test_tofrom_mapping_rename_roundtrip(self, cosmo, to_format, from_format):
+        """Test roundtrip in ``to/from_mapping()`` with ``rename``."""
         to_rename = {"name": "cosmo_name"}
         m = to_format("mapping", rename=to_rename)
 
-        assert "name" not in m.keys()
-        assert "cosmo_name" in m.keys()
+        assert "name" not in m
+        assert "cosmo_name" in m
 
         # Wrong names = error
         with pytest.raises(TypeError, match="there are unused parameters"):