Add type hints and configure mypy

Configure mypy as a pre-commit hook and add all type hints necessary for
mypy to pass.

There's still more work to be done here typing a lot more code, but this
provides a clear starting point.

.pre-commit-config.yaml

@@ -22,3 +22,13 @@ repos:
     hooks:
       - id: isort
         name: isort (python)
+  - repo: https://github.com/pre-commit/mirrors-mypy
+    rev: "v0.910"
+    hooks:
+      - id: mypy
+        files: vdirsyncer/.*
+        additional_dependencies:
+          - types-setuptools
+          - types-docutils
+          - types-requests
+          - types-atomicwrites

setup.cfg

@@ -24,3 +24,8 @@ import-order-style = smarkets
 
 [isort]
 force_single_line=true
+
+[mypy]
+ignore_missing_imports = True
+# See https://github.com/python/mypy/issues/7511:
+warn_no_return = False

setup.py

@@ -25,7 +25,7 @@ requirements = [
 
 class PrintRequirements(Command):
     description = "Prints minimal requirements"
-    user_options = []
+    user_options: list = []
 
     def initialize_options(self):
         pass

tests/system/cli/test_discover.py

@@ -1,5 +1,6 @@
 import json
 from textwrap import dedent
+from typing import List
 
 import pytest
 
@@ -208,6 +209,12 @@ def test_collection_required(a_requires, b_requires, tmpdir, runner, monkeypatch
                 assert not kw.get("collection")
                 raise exceptions.CollectionRequired()
 
+        async def get(self, href: str):
+            raise NotImplementedError()
+
+        async def list(self) -> List[tuple]:
+            raise NotImplementedError()
+
     from vdirsyncer.cli.utils import storage_names
 
     monkeypatch.setitem(storage_names._storages, "test", TestStorage)

vdirsyncer/storage/base.py

@@ -1,12 +1,20 @@
 import contextlib
 import functools
+from abc import ABCMeta
+from abc import abstractmethod
+from typing import Iterable
+from typing import List
 from typing import Optional
 
+from vdirsyncer.vobject import Item
+
 from .. import exceptions
 from ..utils import uniq
 
 
 def mutating_storage_method(f):
+    """Wrap a method and fail if the instance is readonly."""
+
     @functools.wraps(f)
     async def inner(self, *args, **kwargs):
         if self.read_only:
@@ -16,8 +24,10 @@ def mutating_storage_method(f):
     return inner
 
 
-class StorageMeta(type):
+class StorageMeta(ABCMeta):
     def __init__(cls, name, bases, d):
+        """Wrap mutating methods to fail if the storage is readonly."""
+
         for method in ("update", "upload", "delete"):
             setattr(cls, method, mutating_storage_method(getattr(cls, method)))
         return super().__init__(name, bases, d)
@@ -48,7 +58,7 @@ class Storage(metaclass=StorageMeta):
 
     # The string used in the config to denote the type of storage. Should be
     # overridden by subclasses.
-    storage_name = None
+    storage_name: str
 
     # The string used in the config to denote a particular instance. Will be
     # overridden during instantiation.
@@ -63,7 +73,7 @@ class Storage(metaclass=StorageMeta):
     read_only = False
 
     # The attribute values to show in the representation of the storage.
-    _repr_attributes = ()
+    _repr_attributes: List[str] = []
 
     def __init__(self, instance_name=None, read_only=None, collection=None):
         if read_only is None:
@@ -121,13 +131,14 @@ class Storage(metaclass=StorageMeta):
             {x: getattr(self, x) for x in self._repr_attributes},
         )
 
-    async def list(self):
+    @abstractmethod
+    async def list(self) -> List[tuple]:
         """
         :returns: list of (href, etag)
         """
-        raise NotImplementedError()
 
-    async def get(self, href):
+    @abstractmethod
+    async def get(self, href: str):
         """Fetch a single item.
 
         :param href: href to fetch
@@ -135,9 +146,8 @@ class Storage(metaclass=StorageMeta):
         :raises: :exc:`vdirsyncer.exceptions.PreconditionFailed` if item can't
             be found.
         """
-        raise NotImplementedError()
 
-    async def get_multi(self, hrefs):
+    async def get_multi(self, hrefs: Iterable[str]):
         """Fetch multiple items. Duplicate hrefs must be ignored.
 
         Functionally similar to :py:meth:`get`, but might bring performance
@@ -152,11 +162,8 @@ class Storage(metaclass=StorageMeta):
             item, etag = await self.get(href)
             yield href, item, etag
 
-    async def has(self, href):
-        """Check if an item exists by its href.
-
-        :returns: True or False
-        """
+    async def has(self, href) -> bool:
+        """Check if an item exists by its href."""
         try:
             await self.get(href)
         except exceptions.PreconditionFailed:
@@ -164,7 +171,7 @@ class Storage(metaclass=StorageMeta):
         else:
             return True
 
-    async def upload(self, item):
+    async def upload(self, item: Item):
         """Upload a new item.
 
         In cases where the new etag cannot be atomically determined (i.e. in
@@ -179,7 +186,7 @@ class Storage(metaclass=StorageMeta):
         """
         raise NotImplementedError()
 
-    async def update(self, href, item, etag):
+    async def update(self, href: str, item: Item, etag):
         """Update an item.
 
         The etag may be none in some cases, see `upload`.
@@ -192,7 +199,7 @@ class Storage(metaclass=StorageMeta):
         """
         raise NotImplementedError()
 
-    async def delete(self, href, etag):
+    async def delete(self, href: str, etag: str):
         """Delete an item by href.
 
         :raises: :exc:`vdirsyncer.exceptions.PreconditionFailed` when item has
@@ -228,21 +235,19 @@ class Storage(metaclass=StorageMeta):
         :param key: The metadata key.
         :return: The metadata or None, if metadata is missing.
         """
-
         raise NotImplementedError("This storage does not support metadata.")
 
     async def set_meta(self, key: str, value: Optional[str]):
-        """Get metadata value for collection/storage.
+        """Set metadata value for collection/storage.
 
         :param key: The metadata key.
         :param value: The value. Use None to delete the data.
         """
-
         raise NotImplementedError("This storage does not support metadata.")
 
 
 def normalize_meta_value(value) -> Optional[str]:
     # `None` is returned by iCloud for empty properties.
     if value is None or value == "None":
-        return
+        return None
     return value.strip() if value else ""

vdirsyncer/storage/dav.py

@@ -2,14 +2,17 @@ import datetime
 import logging
 import urllib.parse as urlparse
 import xml.etree.ElementTree as etree
+from abc import abstractmethod
 from inspect import getfullargspec
 from inspect import signature
 from typing import Optional
+from typing import Type
 
 import aiohttp
 import aiostream
 
 from vdirsyncer.exceptions import Error
+from vdirsyncer.vobject import Item
 
 from .. import exceptions
 from .. import http
@@ -18,7 +21,6 @@ from ..http import USERAGENT
 from ..http import prepare_auth
 from ..http import prepare_client_cert
 from ..http import prepare_verify
-from ..vobject import Item
 from .base import Storage
 from .base import normalize_meta_value
 
@@ -146,11 +148,31 @@ def _fuzzy_matches_mimetype(strict, weak):
 
 
 class Discover:
-    _namespace = None
-    _resourcetype = None
-    _homeset_xml = None
-    _homeset_tag = None
-    _well_known_uri = None
+    @property
+    @abstractmethod
+    def _namespace(self) -> str:
+        pass
+
+    @property
+    @abstractmethod
+    def _resourcetype(self) -> Optional[str]:
+        pass
+
+    @property
+    @abstractmethod
+    def _homeset_xml(self) -> bytes:
+        pass
+
+    @property
+    @abstractmethod
+    def _homeset_tag(self) -> str:
+        pass
+
+    @property
+    @abstractmethod
+    def _well_known_uri(self) -> str:
+        pass
+
     _collection_xml = b"""
     <propfind xmlns="DAV:">
         <prop>
@@ -347,7 +369,7 @@ class CalDiscover(Discover):
 
 class CardDiscover(Discover):
     _namespace = "urn:ietf:params:xml:ns:carddav"
-    _resourcetype = "{%s}addressbook" % _namespace
+    _resourcetype: Optional[str] = "{%s}addressbook" % _namespace
     _homeset_xml = b"""
     <propfind xmlns="DAV:" xmlns:c="urn:ietf:params:xml:ns:carddav">
         <prop>
@@ -434,21 +456,31 @@ class DAVSession:
 
 class DAVStorage(Storage):
     # the file extension of items. Useful for testing against radicale.
-    fileext = None
+    fileext: str
     # mimetype of items
-    item_mimetype = None
-    # XML to use when fetching multiple hrefs.
-    get_multi_template = None
-    # The LXML query for extracting results in get_multi
-    get_multi_data_query = None
-    # The Discover subclass to use
-    discovery_class = None
+    item_mimetype: str
+
+    @property
+    @abstractmethod
+    def get_multi_template(self) -> str:
+        """XML to use when fetching multiple hrefs."""
+
+    @property
+    @abstractmethod
+    def get_multi_data_query(self) -> str:
+        """LXML query for extracting results in get_multi."""
+
+    @property
+    @abstractmethod
+    def discovery_class(self) -> Type[Discover]:
+        """Discover subclass to use."""
+
     # The DAVSession class to use
     session_class = DAVSession
 
     connector: aiohttp.TCPConnector
 
-    _repr_attributes = ("username", "url")
+    _repr_attributes = ["username", "url"]
 
     _property_table = {
         "displayname": ("displayname", "DAV:"),
@@ -466,7 +498,8 @@ class DAVStorage(Storage):
         )
         super().__init__(**kwargs)
 
-    __init__.__signature__ = signature(session_class.__init__)
+    __init__.__signature__ = signature(session_class.__init__)  # type: ignore
+    # See  https://github.com/python/mypy/issues/5958
 
     @classmethod
     async def discover(cls, **kwargs):
@@ -492,7 +525,7 @@ class DAVStorage(Storage):
     def _is_item_mimetype(self, mimetype):
         return _fuzzy_matches_mimetype(self.item_mimetype, mimetype)
 
-    async def get(self, href):
+    async def get(self, href: str):
         ((actual_href, item, etag),) = await aiostream.stream.list(
             self.get_multi([href])
         )
@@ -588,7 +621,7 @@ class DAVStorage(Storage):
         href, etag = await self._put(self._normalize_href(href), item, etag)
         return etag
 
-    async def upload(self, item):
+    async def upload(self, item: Item):
         href = self._get_href(item)
         rv = await self._put(href, item, None)
         return rv
@@ -687,17 +720,14 @@ class DAVStorage(Storage):
             raise exceptions.UnsupportedMetadataError()
 
         xpath = f"{{{namespace}}}{tagname}"
-        data = """<?xml version="1.0" encoding="utf-8" ?>
+        body = f"""<?xml version="1.0" encoding="utf-8" ?>
             <propfind xmlns="DAV:">
                 <prop>
-                    {}
+                    {etree.tostring(etree.Element(xpath), encoding="unicode")}
                 </prop>
             </propfind>
-        """.format(
-            etree.tostring(etree.Element(xpath), encoding="unicode")
-        ).encode(
-            "utf-8"
-        )
+        """
+        data = body.encode("utf-8")
 
         headers = self.session.get_default_headers()
         headers["Depth"] = "0"

vdirsyncer/storage/filesystem.py

@@ -20,7 +20,7 @@ logger = logging.getLogger(__name__)
 class FilesystemStorage(Storage):
 
     storage_name = "filesystem"
-    _repr_attributes = ("path",)
+    _repr_attributes = ["path"]
 
     def __init__(
         self,

vdirsyncer/storage/google.py

@@ -36,7 +36,8 @@ class GoogleSession(dav.DAVSession):
         client_id,
         client_secret,
         url=None,
-        connector: aiohttp.BaseConnector = None,
+        *,
+        connector: aiohttp.BaseConnector,
     ):
         if not have_oauth2:
             raise exceptions.UserError("aiohttp-oauthlib not installed")
@@ -172,7 +173,7 @@ class GoogleCalendarStorage(dav.CalDAVStorage):
     # This is ugly: We define/override the entire signature computed for the
     # docs here because the current way we autogenerate those docs are too
     # simple for our advanced argspec juggling in `vdirsyncer.storage.dav`.
-    __init__._traverse_superclass = base.Storage
+    __init__._traverse_superclass = base.Storage  # type: ignore
 
 
 class GoogleContactsStorage(dav.CardDAVStorage):
@@ -187,7 +188,7 @@ class GoogleContactsStorage(dav.CardDAVStorage):
         url = "https://www.googleapis.com/.well-known/carddav"
         scope = ["https://www.googleapis.com/auth/carddav"]
 
-    class discovery_class(dav.CardDAVStorage.discovery_class):
+    class discovery_class(dav.CardDiscover):
         # Google CardDAV doesn't return any resourcetype prop.
         _resourcetype = None
 
@@ -207,4 +208,4 @@ class GoogleContactsStorage(dav.CardDAVStorage):
     # This is ugly: We define/override the entire signature computed for the
     # docs here because the current way we autogenerate those docs are too
     # simple for our advanced argspec juggling in `vdirsyncer.storage.dav`.
-    __init__._traverse_superclass = base.Storage
+    __init__._traverse_superclass = base.Storage  # type: ignore

vdirsyncer/storage/http.py

@@ -16,7 +16,7 @@ from .base import Storage
 class HttpStorage(Storage):
     storage_name = "http"
     read_only = True
-    _repr_attributes = ("username", "url")
+    _repr_attributes = ["username", "url"]
     _items = None
 
     # Required for tests.

vdirsyncer/storage/singlefile.py

@@ -36,7 +36,7 @@ def _writing_op(f):
 
 class SingleFileStorage(Storage):
     storage_name = "singlefile"
-    _repr_attributes = ("path",)
+    _repr_attributes = ["path"]
 
     _write_mode = "wb"
     _append_mode = "ab"

vdirsyncer/utils.py

@@ -3,6 +3,7 @@ import os
 import sys
 import uuid
 from inspect import getfullargspec
+from typing import Callable
 
 from . import exceptions
 
@@ -25,7 +26,7 @@ def expand_path(p: str) -> str:
     return p
 
 
-def split_dict(d: dict, f: callable):
+def split_dict(d: dict, f: Callable):
     """Puts key into first dict if f(key), otherwise in second dict"""
     a = {}
     b = {}