Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rudimentary mypy annotations #971

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions .coveragerc
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,6 @@ omit =

[report]
include = piptools/*, tests/*
exclude_lines =
# Don't complain about typing imports
if MYPY:
7 changes: 7 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,10 @@ repos:
- id: bandit
language_version: python3
exclude: ^tests/
- repo: https://github.com/pre-commit/mirrors-mypy
rev: v0.740
hooks:
- id: mypy
language_version: python3
# To prevent "Duplicate module named 'setup'" error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was arguing about this with Anthony a while back. Still now sure about the best way to prevent this: https://github.com/pre-commit/mirrors-mypy/issues/5.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. But then, I think, I've had problems with that too. It's up to you atm.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's nothing wrong with this exception since packages are raw test fixtures. I'd prefer to leave it as is and move on. It's unlikely that type checking in packages could be any useful.

exclude: ^tests/test_data/packages/
2 changes: 2 additions & 0 deletions piptools/_compat/typing.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# The pattern is taken from https://github.com/python/mypy/issues/3216
MYPY = False
22 changes: 21 additions & 1 deletion piptools/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,23 @@

from pip._vendor.packaging.requirements import Requirement

from ._compat.typing import MYPY
from .exceptions import PipToolsError
from .locations import CACHE_DIR
from .utils import as_tuple, key_from_req, lookup_table

if MYPY:
from typing import Dict, List, Optional, Set, Tuple
from ._compat import InstallRequirement


class CorruptCacheError(PipToolsError):
def __init__(self, path):
# type: (str) -> None
self.path = path

def __str__(self):
# type: () -> str
lines = [
"The dependency cache seems to have been corrupted.",
"Inspect, or delete, the following file:",
Expand All @@ -26,6 +33,7 @@ def __str__(self):


def read_cache_file(cache_file_path):
# type: (str) -> Dict[str, dict]
with open(cache_file_path, "r") as cache_file:
try:
doc = json.load(cache_file)
Expand All @@ -49,7 +57,10 @@ class DependencyCache(object):
Where X.Y indicates the Python version.
"""

_cache = None # type: Dict[str, dict]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you move it from instance to class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fixes the following complainings:

piptools/cache.py:82: error: Incompatible return value type (got "None", expected "Dict[str, Dict[Any, Any]]")
piptools/cache.py:109: error: Incompatible types in assignment (expression has type "Dict[str, Dict[Any, Any]]", variable has type "None")
piptools/cache.py:111: error: Incompatible types in assignment (expression has type "Dict[<nothing>, <nothing>]", variable has type "None")
piptools/cache.py:122: error: Incompatible types in assignment (expression has type "Dict[<nothing>, <nothing>]", variable has type "None")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you completely changed the semantics with this move, which may cause unwanted side-effects. Why didn't you just add an annotation in the initializer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried at first:

self._cache = None  # type: Dict[str, dict]

but

piptools/cache.py:71: error: Incompatible types in assignment (expression has type "None", variable has type "Dict[str, Dict[Any, Any]]")

then

self._cache = None  # type: Optional[Dict[str, dict]]

but

piptools/cache.py:82: error: Incompatible return value type (got "Optional[Dict[str, Dict[Any, Any]]]", expected "Dict[str, Dict[Any, Any]]")

I've found the solution and now can't find a reference since I made changes a month ago. I should have fixed it in a separate well-described commit with the reference 😒

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which may cause unwanted side-effects.

Yeah, now _cache is in DependencyCache.__dict__.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might a design problem also.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from typings import Optional, Dict

class Cache:
    def __init__(self):
        # type: () -> None
        self._cache = None  # type: Optional[Dict[str, dict]]

    @property
    def cache(self):
        # type: () -> Dict[str, dict]
        if self._cache is None:
            self.read_cache()
        return self._cache

    def read_cache(self):
        # type: () -> None
        self._cache = {"cache": {}}
$ mypy t.py
Success: no issues found in 1 source file

🤷‍♂

Copy link
Member Author

@atugushev atugushev Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've refactored property(cache) and read_cache to assign explicitly self._cache and it works:

diff --git piptools/cache.py piptools/cache.py
index 5c5fb34..24327f1 100644
--- piptools/cache.py
+++ piptools/cache.py
@@ -57,7 +57,6 @@ class DependencyCache(object):
     Where X.Y indicates the Python version.
     """

-    _cache = None  # type: Dict[str, dict]

     def __init__(self, cache_dir=None):
         # type: (Optional[str]) -> None
@@ -69,6 +68,7 @@ class DependencyCache(object):
         cache_filename = "depcache-py{}.json".format(py_version)

         self._cache_file = os.path.join(cache_dir, cache_filename)
+        self._cache = None  # type: Optional[Dict[str, dict]]

     @property
     def cache(self):
@@ -78,7 +78,7 @@ class DependencyCache(object):
         lazily loads the cache from disk.
         """
         if self._cache is None:
-            self.read_cache()
+            self._cache = self.read_cache()
         return self._cache

     def as_cache_key(self, ireq):
@@ -103,12 +103,12 @@ class DependencyCache(object):
         return name, "{}{}".format(version, extras_string)

     def read_cache(self):
-        # type: () -> None
+        # type: () -> Dict[str, dict]
         """Reads the cached contents into memory."""
         if os.path.exists(self._cache_file):
-            self._cache = read_cache_file(self._cache_file)
+            return read_cache_file(self._cache_file)
         else:
-            self._cache = {}
+            return {}

     def write_cache(self):
         # type: () -> None
$ mypy .
Success: no issues found in 44 source files

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this approach okay?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks good.


def __init__(self, cache_dir=None):
# type: (Optional[str]) -> None
if cache_dir is None:
cache_dir = CACHE_DIR
if not os.path.isdir(cache_dir):
Expand All @@ -58,10 +69,10 @@ def __init__(self, cache_dir=None):
cache_filename = "depcache-py{}.json".format(py_version)

self._cache_file = os.path.join(cache_dir, cache_filename)
self._cache = None

@property
def cache(self):
# type: () -> Dict[str, dict]
"""
The dictionary that is the actual in-memory cache. This property
lazily loads the cache from disk.
Expand All @@ -71,6 +82,7 @@ def cache(self):
return self._cache

def as_cache_key(self, ireq):
# type: (InstallRequirement) -> Tuple[str, str]
"""
Given a requirement, return its cache key. This behavior is a little weird
in order to allow backwards compatibility with cache files. For a requirement
Expand All @@ -91,37 +103,44 @@ def as_cache_key(self, ireq):
return name, "{}{}".format(version, extras_string)

def read_cache(self):
# type: () -> None
"""Reads the cached contents into memory."""
if os.path.exists(self._cache_file):
self._cache = read_cache_file(self._cache_file)
else:
self._cache = {}

def write_cache(self):
# type: () -> None
"""Writes the cache to disk as JSON."""
doc = {"__format__": 1, "dependencies": self._cache}
with open(self._cache_file, "w") as f:
json.dump(doc, f, sort_keys=True)

def clear(self):
# type: () -> None
self._cache = {}
self.write_cache()

def __contains__(self, ireq):
# type: (InstallRequirement) -> bool
pkgname, pkgversion_and_extras = self.as_cache_key(ireq)
return pkgversion_and_extras in self.cache.get(pkgname, {})

def __getitem__(self, ireq):
# type: (InstallRequirement) -> List[str]
pkgname, pkgversion_and_extras = self.as_cache_key(ireq)
return self.cache[pkgname][pkgversion_and_extras]

def __setitem__(self, ireq, values):
# type: (InstallRequirement, List[str]) -> None
pkgname, pkgversion_and_extras = self.as_cache_key(ireq)
self.cache.setdefault(pkgname, {})
self.cache[pkgname][pkgversion_and_extras] = values
self.write_cache()

def reverse_dependencies(self, ireqs):
# type: (List[InstallRequirement]) -> Dict[str, Set[str]]
"""
Returns a lookup table of reverse dependencies for all the given ireqs.

Expand All @@ -134,6 +153,7 @@ def reverse_dependencies(self, ireqs):
return self._reverse_dependencies(ireqs_as_cache_values)

def _reverse_dependencies(self, cache_keys):
# type: (List[Tuple[str, str]]) -> Dict[str, Set[str]]
"""
Returns a lookup table of reverse dependencies for all the given cache keys.

Expand Down
7 changes: 7 additions & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,10 @@ line_length = 88
multi_line_output = 3
default_section=THIRDPARTY
known_first_party = piptools, tests, examples

[mypy]
follow_imports = silent
ignore_missing_imports = True

[mypy-tests.*]
ignore_errors = True