Repository: skypyproject/skypy

Issue:
SkyPyLoader parses names as non-string variables
The SkyPyLoader parses variable names using the same logic that it parses their values. As a result, variable names can take non-string types including int, float and astropy Quantity. Consider the config file:

```yml
# config.yml
42_km: 42 km
43 km: 43 km
```

``` python
>>> from skypy.pipeline import load_skypy_yaml
>>> config = load_skypy_yaml("./config.yml")
>>> print(config)

{'42_km': <Quantity 42. km>, <Quantity 43. km>: <Quantity 43. km>}
```

For the first entry, the key "42_km" is correctly interpreted as a string. However for the second entry  the key "43 km" is interpreted as an astropy Quantity by the same logic that applies to its corresponding value. This is problematic because `Pipeline.__getitem__` assumes all variable names in `state` are strings.

@ntessore pointed out that pyyaml will not let you apply different logic to keys and values when parsing files and therefore suggested that instead we enforce all variable names must be strings within the Pipeline class.

Discussion:
JonathanDHarris: Thanks @rrjbca .  I moved the import and the methods off the `SkyPyLoader` class.  I was trying to neaten it up but I have to be honest- I don't entirely know what the preferred style is for Python and I was partly following PyCharm's suggestions when I did this.  I can move them back how you wrote them if that's correct.

Patch:
diff --git a/skypy/pipeline/_config.py b/skypy/pipeline/_config.py
index f2aa6a0..1d33eea 100644
--- a/skypy/pipeline/_config.py
+++ b/skypy/pipeline/_config.py
@@ -3,6 +3,7 @@ from importlib import import_module
 import yaml
 import re
 from astropy.units import Quantity
+from collections.abc import Mapping
 
 __all__ = [
     'load_skypy_yaml',
@@ -22,6 +23,26 @@ def import_function(qualname):
     return function
 
 
+def validate_keys(config):
+    for k in config.keys():
+        if not isinstance(k, str):
+            raise ValueError(f"Invalid key found in config. {k} is not a string. "
+                             f"Either rename this value or wrap it in quotes.")
+
+
+def validate_config(config):
+    # Check each key at the current depth is a string
+    validate_keys(config)
+    for v in config.values():
+        # If any values are dictionaries, recurse
+        if isinstance(v, Mapping):
+            validate_config(v)
+        # If any values are tuples (i.e. function calls) validate kwargs
+        if isinstance(v, tuple) and len(v) > 1 and isinstance(v[1], Mapping):
+            validate_keys(v[1])
+    return config
+
+
 class SkyPyLoader(yaml.SafeLoader):
     '''custom YAML loader class with SkyPy extensions'''
 
@@ -30,7 +51,8 @@ class SkyPyLoader(yaml.SafeLoader):
         '''load the first YAML document from stream'''
         loader = cls(stream)
         try:
-            return loader.get_single_data()
+            single_data = loader.get_single_data()
+            return validate_config(single_data if single_data else {})
         finally:
             loader.dispose()
 


Test patch:
diff --git a/skypy/pipeline/tests/data/numeric_key.yml b/skypy/pipeline/tests/data/numeric_key.yml
new file mode 100644
index 0000000..9527312
--- /dev/null
+++ b/skypy/pipeline/tests/data/numeric_key.yml
@@ -0,0 +1,1 @@
+43 km: 43 km
diff --git a/skypy/pipeline/tests/data/numeric_kwarg.yml b/skypy/pipeline/tests/data/numeric_kwarg.yml
new file mode 100644
index 0000000..7c8de18
--- /dev/null
+++ b/skypy/pipeline/tests/data/numeric_kwarg.yml
@@ -0,0 +1,4 @@
+tables:
+  test_table_1:
+    test_column_1: !numpy.random.uniform
+      10: 10
diff --git a/skypy/pipeline/tests/data/numeric_nested_key.yml b/skypy/pipeline/tests/data/numeric_nested_key.yml
new file mode 100644
index 0000000..fafa53d
--- /dev/null
+++ b/skypy/pipeline/tests/data/numeric_nested_key.yml
@@ -0,0 +1,4 @@
+tables:
+  test_table_1:
+    10 km: !numpy.random.uniform
+      size: 10
diff --git a/skypy/pipeline/tests/test_config.py b/skypy/pipeline/tests/test_config.py
index 525c616..b789b15 100644
--- a/skypy/pipeline/tests/test_config.py
+++ b/skypy/pipeline/tests/test_config.py
@@ -41,3 +41,27 @@ def test_yaml_quantities():
 
     assert config['42_km'] == units.Quantity('42 km')
     assert config['1_deg2'] == units.Quantity('1 deg2')
+
+
+def test_keys_must_be_strings():
+    # config with key that doesn't parse as String.
+    filename = get_pkg_data_filename('data/numeric_key.yml')
+    with pytest.raises(ValueError) as e:
+        load_skypy_yaml(filename)
+    assert("Invalid key found in config" in e.value.args[0])
+
+
+def test_nested_keys_must_be_strings():
+    # config with nested key that doesn't parse as String.
+    filename = get_pkg_data_filename('data/numeric_nested_key.yml')
+    with pytest.raises(ValueError) as e:
+        load_skypy_yaml(filename)
+    assert("Invalid key found in config" in e.value.args[0])
+
+
+def test_kwarg_must_be_strings():
+    # config with function kwarg name that doesn't parse as String.
+    filename = get_pkg_data_filename('data/numeric_kwarg.yml')
+    with pytest.raises(ValueError) as e:
+        load_skypy_yaml(filename)
+    assert("Invalid key found in config" in e.value.args[0])