Repository: popsim-consortium/demes-python (language: python)

Issue:
clean up the exported names
We should use `__all__` to only export names that are part of the public API. Also see mypy --strict output in #323, where the `Module has no attribute "foo"` suggests where we might consider explicit exports.


Discussion / hints:
Serious question: is it actually possible to satisfy mypy?  In my experience, every warning or two that I silence seems to open up other cans of worms.
> Serious question: is it actually possible to satisfy mypy? In my experience, every warning or two that I silence seems to open up other cans of worms.

Scanning the `mypy --strict` output, I think there's a bunch of stuff that could be fixed quite easily. We can just ignore the hard bits. But given that we want demes to be used widely, I think it's worth making some effort to have good type annotations so that downstream projects can benefit from type checking too.

Apart from attrs classes, we're mostly using stock Python types (float, int, str) and simple containers (list, dict, tuple), so annotations for function params and return values shouldn't be too onerous. Of course, typing support is not as good in Python 3.6 as in Python 3.9, so there will be some constraints (e.g. having to say `typing.List[float]` instead of just `list[float]`).

Fix patch:
diff --git a/demes/__init__.py b/demes/__init__.py
index ae03846c..5de76a98 100644
--- a/demes/__init__.py
+++ b/demes/__init__.py
@@ -1,5 +1,3 @@
-# flake8: noqa: F401
-
 __version__ = "undefined"
 try:
     from . import _version
@@ -31,3 +29,35 @@
     dump_all,
 )
 from .ms import from_ms
+
+__all__ = [
+    "Builder",
+    "Epoch",
+    "AsymmetricMigration",
+    "Pulse",
+    "Deme",
+    "Graph",
+    "Split",
+    "Branch",
+    "Merge",
+    "Admix",
+    "load_asdict",
+    "loads_asdict",
+    "load",
+    "loads",
+    "load_all",
+    "dump",
+    "dumps",
+    "dump_all",
+    "from_ms",
+]
+
+
+# Override the symbols that are returned when calling dir(<module-name>).
+# https://www.python.org/dev/peps/pep-0562/
+# We do this because the Python REPL and IPython notebooks ignore __all__
+# when providing autocomplete suggestions. They instead rely on dir().
+# By not showing internal symbols in the dir() output, we reduce the chance
+# that users rely on non-public features.
+def __dir__():
+    return sorted(__all__)
diff --git a/demes/hypothesis_strategies.py b/demes/hypothesis_strategies.py
index 23478626..718d3e42 100644
--- a/demes/hypothesis_strategies.py
+++ b/demes/hypothesis_strategies.py
@@ -8,6 +8,12 @@
 
 import demes
 
+__all__ = ["graphs"]
+
+
+def __dir__():
+    return sorted(__all__)
+
 
 def prec32(x):
     """truncate x to the nearest single-precision floating point number"""
diff --git a/setup.cfg b/setup.cfg
index 44f2324b..1c5e39f8 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -39,6 +39,10 @@ extend-exclude = docs/_build
 # black-compatible settings
 max-line-length = 88
 extend-ignore = E203, W503
+# There's no way to ignore specific warnings in the files themselves.
+# "flake8: noqa: F401" on its own line will just ignore all warnings.
+per-file-ignores =
+    tests/test_import_visibility.py:F403,F405
 
 [mypy]
 files = demes, tests


Test patch:
diff --git a/tests/test_import_visibility.py b/tests/test_import_visibility.py
new file mode 100644
index 00000000..931790b2
--- /dev/null
+++ b/tests/test_import_visibility.py
@@ -0,0 +1,96 @@
+import sys
+import pathlib
+import tempfile
+
+import pytest
+from demes import *
+
+
+def test_builder():
+    b = Builder()
+    b.add_deme("A", epochs=[dict(start_size=100)])
+    b.resolve()
+
+
+def test_dumps_and_loads():
+    b = Builder()
+    b.add_deme("A", epochs=[dict(start_size=100)])
+    graph1 = b.resolve()
+    dump_str = dumps(graph1)
+    graph2 = loads(dump_str)
+    graph1.assert_close(graph2)
+
+
+def test_dump_and_load():
+    b = Builder()
+    b.add_deme("A", epochs=[dict(start_size=100)])
+    graph1 = b.resolve()
+    with tempfile.TemporaryDirectory() as tmpdir:
+        tmpfile = pathlib.Path(tmpdir) / "temp.yaml"
+        dump(graph1, tmpfile)
+        graph2 = load(tmpfile)
+    graph1.assert_close(graph2)
+
+
+def test_public_symbols():
+    Builder
+    Epoch
+    AsymmetricMigration
+    Pulse
+    Deme
+    Graph
+    Split
+    Branch
+    Merge
+    Admix
+
+    load_asdict
+    loads_asdict
+    load
+    loads
+    load_all
+    dump
+    dumps
+    dump_all
+
+    from_ms
+
+
+def test_nonpublic_symbols():
+    with pytest.raises(NameError):
+        demes
+    with pytest.raises(NameError):
+        load_dump
+    with pytest.raises(NameError):
+        ms
+    with pytest.raises(NameError):
+        prec32
+
+
+PY36 = sys.version_info[0:2] < (3, 7)
+
+
+@pytest.mark.xfail(PY36, reason="__dir__ does nothing on Python 3.6", strict=True)
+def test_demes_dir():
+    import demes
+
+    dir_demes = set(dir(demes))
+    assert "load" in dir_demes
+    assert "dump" in dir_demes
+    assert "loads" in dir_demes
+    assert "dumps" in dir_demes
+
+    assert "demes" not in dir_demes
+    assert "load_dump" not in dir_demes
+    assert "ms" not in dir_demes
+    assert "graphs" not in dir_demes
+    assert "prec32" not in dir_demes
+
+
+@pytest.mark.xfail(PY36, reason="__dir__ does nothing on Python 3.6", strict=True)
+def test_hypothesis_strategy_dir():
+    import demes.hypothesis_strategies
+
+    dir_demes_hs = set(dir(demes.hypothesis_strategies))
+    assert "graphs" in dir_demes_hs
+    assert "prec32" not in dir_demes_hs