From f3714fc493e69be92f930daf68ab93c80444ea14 Mon Sep 17 00:00:00 2001 From: Hugo Osvaldo Barrera Date: Sun, 25 Jul 2021 21:17:16 +0200 Subject: [PATCH] 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 | 10 ++++ setup.cfg | 5 ++ setup.py | 2 +- tests/system/cli/test_discover.py | 7 +++ vdirsyncer/storage/base.py | 45 +++++++++-------- vdirsyncer/storage/dav.py | 82 +++++++++++++++++++++---------- vdirsyncer/storage/filesystem.py | 2 +- vdirsyncer/storage/google.py | 9 ++-- vdirsyncer/storage/http.py | 2 +- vdirsyncer/storage/singlefile.py | 2 +- vdirsyncer/utils.py | 3 +- 11 files changed, 114 insertions(+), 55 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 6364238..d4ee139 100644 --- a/.pre-commit-config.yaml +++ b/.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 diff --git a/setup.cfg b/setup.cfg index 59f3793..37065bd 100644 --- a/setup.cfg +++ b/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 diff --git a/setup.py b/setup.py index b42b2e7..99f8de7 100644 --- a/setup.py +++ b/setup.py @@ -25,7 +25,7 @@ requirements = [ class PrintRequirements(Command): description = "Prints minimal requirements" - user_options = [] + user_options: list = [] def initialize_options(self): pass diff --git a/tests/system/cli/test_discover.py b/tests/system/cli/test_discover.py index 11083ef..d47182e 100644 --- a/tests/system/cli/test_discover.py +++ b/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) diff --git a/vdirsyncer/storage/base.py b/vdirsyncer/storage/base.py index aff3cdf..a9061cc 100644 --- a/vdirsyncer/storage/base.py +++ b/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 "" diff --git a/vdirsyncer/storage/dav.py b/vdirsyncer/storage/dav.py index 1d699dc..9cb0d90 100644 --- a/vdirsyncer/storage/dav.py +++ b/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""" @@ -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""" @@ -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 = """ + body = f""" - {} + {etree.tostring(etree.Element(xpath), encoding="unicode")} - """.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" diff --git a/vdirsyncer/storage/filesystem.py b/vdirsyncer/storage/filesystem.py index 6656705..cb4b307 100644 --- a/vdirsyncer/storage/filesystem.py +++ b/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, diff --git a/vdirsyncer/storage/google.py b/vdirsyncer/storage/google.py index de18c2c..210f999 100644 --- a/vdirsyncer/storage/google.py +++ b/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 diff --git a/vdirsyncer/storage/http.py b/vdirsyncer/storage/http.py index f2904a9..4e3fc79 100644 --- a/vdirsyncer/storage/http.py +++ b/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. diff --git a/vdirsyncer/storage/singlefile.py b/vdirsyncer/storage/singlefile.py index 800998a..7edfc3a 100644 --- a/vdirsyncer/storage/singlefile.py +++ b/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" diff --git a/vdirsyncer/utils.py b/vdirsyncer/utils.py index bd09cd8..ad96598 100644 --- a/vdirsyncer/utils.py +++ b/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 = {}