Add rudimentary mypy annotations #971

atugushev · 2019-10-26T06:37:27Z

What's done

updated setup.cfg
added mypy hook to pre-commit
added typings for the cache module

Refs: #972.

codecov · 2019-10-26T08:02:20Z

Codecov Report

Merging #971 into master will decrease coverage by 0.33%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #971      +/-   ##
==========================================
- Coverage   99.31%   98.97%   -0.34%     
==========================================
  Files          35       35              
  Lines        2329     2338       +9     
  Branches      301      303       +2     
==========================================
+ Hits         2313     2314       +1     
- Misses          8       15       +7     
- Partials        8        9       +1

Impacted Files	Coverage Δ
piptools/cache.py	`100% <100%> (ø)`	⬆️
piptools/__init__.py	`66.66% <0%> (-33.34%)`	⬇️
tests/utils.py	`66.66% <0%> (-33.34%)`	⬇️
tests/test_repository_pypi.py	`97.82% <0%> (-2.18%)`	⬇️
piptools/repositories/pypi.py	`94.41% <0%> (-1%)`	⬇️
piptools/resolver.py	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02fb529...1a9b4be. Read the comment docs.

webknjaz · 2019-10-27T16:27:30Z

.pre-commit-config.yaml

+    hooks:
+    - id: mypy
+      language_version: python3
+      # To prevent "Duplicate module named 'setup'" error


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.

Would you suggest this solution? https://github.com/pre-commit/mirrors-mypy/issues/5#issuecomment-515841288

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

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.

webknjaz · 2019-10-27T16:28:53Z

piptools/cache.py

@@ -49,7 +57,10 @@ class DependencyCache(object):
    Where X.Y indicates the Python version.
    """

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


Why did you move it from instance to class?

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")

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?

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 😒

which may cause unwanted side-effects.

Yeah, now _cache is in DependencyCache.__dict__.

There might a design problem also.

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

🤷‍♂

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

Is this approach okay?

Yeah, looks good.

codingjoe · 2019-10-29T01:36:08Z

@atugushev general question, why don't we use proper PEP484 type hints? They have been implemented in Python 3.5. We don't need to support Python 3.4 since it reached EoL in March this year.

If we really want to use comments for type annotations, I would recommend going with Google style doc strings.

atugushev · 2019-10-29T02:02:22Z

@codingjoe type annotations were presented in Python 3, and they have not been backported to Python 2. The general solution is to use type comments for projects which support legacy Python. When we finally drop Python 2 support, we can convert type comments to the proper Python 3 style type annotations using pytype (or similar).

codingjoe · 2019-10-29T02:27:11Z

@codingjoe type annotations were presented in Python 3, and they have not been backported to Python 2. The general solution is to use type comments for projects which support legacy Python. When we finally drop Python 2 support, we can convert type comments to the proper Python 3 style type annotations using pytype (or similar).

Why don't we just drop Python 2 support? There's only two months left until EoL. Should we find a security vulnerability we can backport that patch on an older major version of pip-tools.

It's probably going to take 2 months until we removed all Python 2 specific code anyways, so by the time we release a new major version, Python 2 is finally going to be a thing of the past.

I mean, if Django, which has over 100x more users, dropped Python 2 main stream support years ago, why are we having such a hard time? Just saying ;)

atugushev · 2019-10-29T16:41:21Z

@codingjoe

Why don't we just drop Python 2 support?

Perhaps, it makes sense to drop support Python 2 when pip does. Discussion about a final date is still on, though.

atugushev · 2019-10-29T17:26:48Z

@codingjoe but, personally, I'm in favor of dropping Python 2 support after 2020-01-01 and add pinned pip to the latest minor version as a dependency.

codingjoe · 2019-10-30T00:00:01Z

@atugushev is that statistic for the latest version? Also, I believe a lot of this will be package mirrors. Then again, it doesn't matter. You can still use an old version, and we can patch an old version until end of year. All I want to achieve is to save you time and effort to implement something, that we could really do a lot better in Python 3 only.

atugushev · 2019-10-30T04:49:38Z

@codingjoe

@atugushev is that statistic for the latest version? Also, I believe a lot of this will be package mirrors.

There are all the pip-tools versions.

Then again, it doesn't matter. You can still use an old version, and we can patch an old version until end of year. All I want to achieve is to save you time and effort to implement something, that we could really do a lot better in Python 3 only.

That's why I've added the alternative solution to the #972 😉

atugushev · 2020-06-28T06:31:12Z

Close this due to #972 (comment).

atugushev added 2 commits October 26, 2019 12:07

Add mypy hook to pre-commit config

c17a505

Add typings for the cache module

21b7130

atugushev added the maintenance Related to maintenance processes label Oct 26, 2019

atugushev mentioned this pull request Oct 26, 2019

Type annotations #972

Closed

16 tasks

Exclude typing imports from coverage

1a9b4be

atugushev requested review from codingjoe, webknjaz, aleksihakli and blueyed October 27, 2019 16:20

webknjaz reviewed Oct 27, 2019

View reviewed changes

aleksihakli removed their request for review October 28, 2019 11:35

codingjoe removed their request for review April 21, 2020 11:05

atugushev closed this Jun 28, 2020

atugushev deleted the mypy branch June 28, 2020 06:31

atugushev mentioned this pull request Dec 16, 2020

Use mypy for static type checking #1275

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rudimentary mypy annotations #971

Add rudimentary mypy annotations #971

atugushev commented Oct 26, 2019 •

edited

codecov bot commented Oct 26, 2019 •

edited

webknjaz Oct 27, 2019

atugushev Oct 27, 2019

webknjaz Oct 27, 2019

atugushev Oct 28, 2019

webknjaz Oct 27, 2019

atugushev Oct 27, 2019

webknjaz Oct 27, 2019

atugushev Oct 28, 2019

atugushev Oct 28, 2019

atugushev Oct 28, 2019

atugushev Oct 28, 2019

atugushev Oct 28, 2019 •

edited

atugushev Oct 29, 2019

webknjaz Apr 21, 2020

codingjoe commented Oct 29, 2019 •

edited

atugushev commented Oct 29, 2019 •

edited

codingjoe commented Oct 29, 2019 •

edited

atugushev commented Oct 29, 2019 •

edited

atugushev commented Oct 29, 2019

codingjoe commented Oct 30, 2019 •

edited

atugushev commented Oct 30, 2019

atugushev commented Jun 28, 2020

Add rudimentary mypy annotations #971

Add rudimentary mypy annotations #971

Conversation

atugushev commented Oct 26, 2019 • edited

What's done

codecov bot commented Oct 26, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atugushev Oct 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingjoe commented Oct 29, 2019 • edited

atugushev commented Oct 29, 2019 • edited

codingjoe commented Oct 29, 2019 • edited

atugushev commented Oct 29, 2019 • edited

atugushev commented Oct 29, 2019

codingjoe commented Oct 30, 2019 • edited

atugushev commented Oct 30, 2019

atugushev commented Jun 28, 2020

atugushev commented Oct 26, 2019 •

edited

codecov bot commented Oct 26, 2019 •

edited

atugushev Oct 28, 2019 •

edited

codingjoe commented Oct 29, 2019 •

edited

atugushev commented Oct 29, 2019 •

edited

codingjoe commented Oct 29, 2019 •

edited

atugushev commented Oct 29, 2019 •

edited

codingjoe commented Oct 30, 2019 •

edited