Merge branch 'more-type-checking'

.flake8

@@ -10,6 +10,7 @@ select =
 
 exclude =
     .git,
+    .venv,
     __pycache__,
     build,
     dist,

.github/workflows/ci.yaml

@@ -31,11 +31,11 @@ jobs:
       - name: Setup test environment
         run: |
           # Install required external programs for --native.
-          conda create -p ~/ci -c bioconda \
+          conda create -p ~/ci -c conda-forge -c bioconda \
               fasttree \
               iqtree \
               mafft \
-              nodejs=10 \
+              nodejs=12 \
               raxml \
               vcftools
 

doc/development.md

@@ -67,11 +67,19 @@ During development you can run static type checks using [mypy][]:
     $ mypy nextstrain
     # No output is good!
 
-There are also many [editor integrations for mypy][].
+and [pyright][]:
+
+    $ npx pyright
+    ...
+    Found 40 source files
+    0 errors, 0 warnings, 0 infos
+    Completed in 2sec
+
+There are also many [editor integrations for mypy][], and Pyright is integrated
+into VS Code's Python support.
 
 The [`typing_extensions`][] module should be used for features added to the
-standard `typings` module after 3.6.  (Currently this isn't necessary since we
-don't use those features.)
+standard `typings` module after 3.6.
 
 We also use [Flake8][] for some static analysis checks focusing on runtime
 safety and correctness.  You can run them like this:
@@ -86,6 +94,7 @@ safety and correctness.  You can run them like this:
 [twine]: https://pypi.org/project/twine
 [type annotations]: https://www.python.org/dev/peps/pep-0484/
 [mypy]: http://mypy-lang.org/
+[pyright]: https://github.com/microsoft/pyright
 [editor integrations for mypy]: https://github.com/python/mypy#ide--linter-integrations
 [`typing_extensions`]: https://pypi.org/project/typing-extensions
 [Flake8]: https://flake8.pycqa.org

mypy.ini

@@ -2,6 +2,14 @@
 # We currently aim for compat with 3.6.
 python_version = 3.6
 
+# Check function bodies which don't have a typed signature. This prevents a
+# single untyped function from poisoning other typed functions in a call chain.
+check_untyped_defs = True
+
+# Require functions with an annotated return type to be explicit about
+# potentially returning None (via Optional[…]).
+strict_optional = False
+
 # In the future maybe we can contribute typing stubs for these modules (either
 # complete stubs in the python/typeshed repo or partial stubs just in
 # this repo), but for now that's more work than we want to invest.  These

nextstrain/cli/argparse.py

@@ -145,7 +145,7 @@ class AppendOverwriteDefault(Action):
     def __call__(self, parser, namespace, value, option_string = None):
         current = getattr(namespace, self.dest, None)
 
-        if current is parser.get_default(self.dest):
+        if current is parser.get_default(self.dest) or current is None:
             current = []
 
         setattr(namespace, self.dest, [*current, value])

nextstrain/cli/authn.py

@@ -56,6 +56,8 @@ def login(username: str, password: str) -> User:
     _save_tokens(session)
     print(f"Credentials saved to {config.SECRETS}.")
 
+    assert session.id_claims
+
     return User(session.id_claims)
 
 
@@ -99,6 +101,8 @@ def current_user() -> Optional[User]:
     except (cognito.TokenError, cognito.NotAuthorizedError):
         return None
 
+    assert session.id_claims
+
     return User(session.id_claims)
 
 

nextstrain/cli/command/check_setup.py

@@ -61,7 +61,7 @@ def run(opts: Options) -> int:
     print("Testing your setup…")
 
     runner_tests = [
-        (runner, runner.test_setup()) # type: ignore
+        (runner, runner.test_setup())
             for runner in all_runners
     ]
 

nextstrain/cli/command/login.py

@@ -20,7 +20,6 @@
 from functools import partial
 from getpass import getpass
 from os import environ
-from textwrap import dedent
 from ..authn import current_user, login
 from ..errors import UserError
 
@@ -79,11 +78,11 @@ def run(opts):
         print()
     else:
         if opts.username is not None and opts.username != user.username:
-            raise UserError(dedent(f"""\
+            raise UserError(f"""
                 Login requested for {opts.username}, but {user.username} is already logged in.
                 
                 Please logout first if you want to switch users.
-                """).rstrip())
+                """)
 
     print(f"Logged into nextstrain.org as {user.username}.")
     print("Log out with `nextstrain logout`.")

nextstrain/cli/command/remote/delete.py

@@ -4,16 +4,10 @@
 See `nextstrain remote --help` for more information on remote sources.
 """
 
-from urllib.parse import urlparse
-from ...remote import s3
+from ...remote import parse_remote_path
 from ...util import warn
 
 
-SUPPORTED_SCHEMES = {
-    "s3": s3,
-}
-
-
 def register_parser(subparser):
     parser = subparser.add_parser(
         "delete",
@@ -34,15 +28,7 @@ def register_parser(subparser):
 
 
 def run(opts):
-    url = urlparse(opts.remote_path)
-
-    if url.scheme not in SUPPORTED_SCHEMES:
-        warn("Error: Unsupported remote scheme %s://" % url.scheme)
-        warn("")
-        warn("Supported schemes are: %s" % ", ".join(SUPPORTED_SCHEMES))
-        return 1
-
-    remote = SUPPORTED_SCHEMES[url.scheme]
+    remote, url = parse_remote_path(opts.remote_path)
 
     deleted = remote.delete(url, recursively = opts.recursively)
     deleted_count = 0

nextstrain/cli/command/remote/download.py

@@ -18,16 +18,10 @@
 """
 
 from pathlib import Path
-from urllib.parse import urlparse
-from ...remote import s3
+from ...remote import parse_remote_path
 from ...util import warn
 
 
-SUPPORTED_SCHEMES = {
-    "s3": s3,
-}
-
-
 def register_parser(subparser):
     parser = subparser.add_parser("download", help = "Download dataset and narrative files")
 
@@ -54,15 +48,7 @@ def register_parser(subparser):
 
 
 def run(opts):
-    url = urlparse(opts.remote_path)
-
-    if url.scheme not in SUPPORTED_SCHEMES:
-        warn("Error: Unsupported remote scheme %s://" % url.scheme)
-        warn("")
-        warn("Supported schemes are: %s" % ", ".join(SUPPORTED_SCHEMES))
-        return 1
-
-    remote = SUPPORTED_SCHEMES[url.scheme]
+    remote, url = parse_remote_path(opts.remote_path)
 
     if opts.recursively and not opts.local_path.is_dir():
         warn("Local path must be a directory when using --recursively; «%s» is not" % opts.local_path)

nextstrain/cli/command/remote/ls.py

@@ -10,14 +10,7 @@
 See `nextstrain remote --help` for more information on remote sources.
 """
 
-from urllib.parse import urlparse
-from ...remote import s3
-from ...util import warn
-
-
-SUPPORTED_SCHEMES = {
-    "s3": s3,
-}
+from ...remote import parse_remote_path
 
 
 def register_parser(subparser):
@@ -35,15 +28,7 @@ def register_parser(subparser):
 
 
 def run(opts):
-    url = urlparse(opts.remote_path)
-
-    if url.scheme not in SUPPORTED_SCHEMES:
-        warn("Error: Unsupported remote scheme %s://" % url.scheme)
-        warn("")
-        warn("Supported schemes are: %s" % ", ".join(SUPPORTED_SCHEMES))
-        return 1
-
-    remote = SUPPORTED_SCHEMES[url.scheme]
+    remote, url = parse_remote_path(opts.remote_path)
 
     files = remote.ls(url)
 

nextstrain/cli/command/remote/upload.py

@@ -13,14 +13,7 @@
 """
 
 from pathlib import Path
-from urllib.parse import urlparse
-from ...remote import s3
-from ...util import warn
-
-
-SUPPORTED_SCHEMES = {
-    "s3": s3,
-}
+from ...remote import parse_remote_path
 
 
 def register_parser(subparser):
@@ -47,16 +40,9 @@ def register_arguments(parser):
 
 
 def run(opts):
-    url = urlparse(opts.destination)
-
-    if url.scheme not in SUPPORTED_SCHEMES:
-        warn("Error: Unsupported destination scheme %s://" % url.scheme)
-        warn("")
-        warn("Supported schemes are: %s" % ", ".join(SUPPORTED_SCHEMES))
-        return 1
+    remote, url = parse_remote_path(opts.destination)
 
-    remote = SUPPORTED_SCHEMES[url.scheme]
-    files  = [Path(f) for f in opts.files]
+    files = [Path(f) for f in opts.files]
 
     uploads = remote.upload(url, files)
 

nextstrain/cli/errors.py

@@ -1,11 +1,17 @@
 """
 Exception classes for internal use.
 """
+from textwrap import dedent
+
 
 class NextstrainCliError(Exception):
     """Exception base class for all custom :mod:`nextstrain.cli` exceptions."""
     pass
 
 class UserError(NextstrainCliError):
-    def __init__(self, message, *args, **kwargs):
-        super().__init__("Error: " + message, *args, **kwargs)
+    def __init__(self, message):
+        # Remove leading newlines, trailing whitespace, and then indentation
+        # to better support nicely-formatted """multi-line strings""".
+        formatted_message = dedent(message.lstrip("\n").rstrip())
+
+        super().__init__("Error: " + formatted_message)

nextstrain/cli/gzip.py

@@ -32,6 +32,7 @@ def readable(self):
 
     def read(self, size = None):
         assert size != 0
+        assert self.stream
 
         if size is None:
             size = -1
@@ -93,14 +94,17 @@ def writable(self):
         return True
 
     def write(self, data: bytes): # type: ignore[override]
+        assert self.stream and self.__gunzip
         return self.stream.write(self.__gunzip.decompress(data))
 
     def flush(self):
+        assert self.stream and self.__gunzip
         super().flush()
         self.stream.flush()
 
     def close(self):
         if self.stream:
+            assert self.__gunzip
             try:
                 self.stream.write(self.__gunzip.flush())
                 self.stream.close()

nextstrain/cli/remote/__init__.py

@@ -0,0 +1,54 @@
+"""
+Remote destinations and sources for Nextstrain datasets and narratives.
+"""
+
+from pathlib import Path
+from typing import cast, Dict, Iterable, List, Tuple, TYPE_CHECKING
+from urllib.parse import urlparse, ParseResult
+from ..errors import UserError
+from ..types import RemoteModule
+from . import s3 as __s3
+
+
+# While PEP-0544 allows for modules to serve as implementations of Protocols¹,
+# Mypy doesn't currently support it².  Pyright does³, however, so we tell Mypy
+# to "trust us", but let Pyright actually check our work.  Mypy considers the
+# MYPY variable to always be True when evaluating the code, regardless of the
+# assignment below.
+#
+# This bit of type checking chicanery is not ideal, but the benefit of having
+# our module interfaces actually checked by Pyright is worth it.  In the
+# future, we should maybe ditch Mypy in favor of Pyright alone, but I didn't
+# want to put in the due diligence for a full switchover right now.
+#
+#   -trs, 12 August 2021
+#
+# ¹ https://www.python.org/dev/peps/pep-0544/#modules-as-implementations-of-protocols
+# ² https://github.com/python/mypy/issues/5018
+# ³ https://github.com/microsoft/pyright/issues/1341
+#
+MYPY = False
+if TYPE_CHECKING and MYPY:
+    s3 = cast(RemoteModule, __s3)
+else:
+    s3 = __s3
+
+
+SUPPORTED_SCHEMES: Dict[str, RemoteModule] = {
+    "s3": s3,
+}
+
+
+def parse_remote_path(path: str) -> Tuple[RemoteModule, ParseResult]:
+    url = urlparse(path)
+
+    if url.scheme not in SUPPORTED_SCHEMES:
+        raise UserError(f"""
+            Unsupported remote scheme {url.scheme}://
+
+            Supported schemes are: {", ".join(SUPPORTED_SCHEMES)}
+            """)
+
+    remote = SUPPORTED_SCHEMES[url.scheme]
+
+    return remote, url