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 typing hint to httpcache downloadermiddlewares #4243

Merged
merged 25 commits into from Aug 17, 2020

Conversation

grammy-jiang
Copy link
Contributor

@grammy-jiang grammy-jiang commented Dec 18, 2019

This commit adds typing hint to httpcache downloadermiddlewares as a
test for #4041

This commit is a trial for adding typing hint to this project

This commit adds typing hint to httpcache downloadermiddlewares as a
test for scrapy#4041
self.storage = load_object(settings['HTTPCACHE_STORAGE'])(settings)
self.ignore_missing = settings.getbool('HTTPCACHE_IGNORE_MISSING')
self.policy: Union[DummyPolicy, RFC2616Policy] = load_object(settings['HTTPCACHE_POLICY'])(settings)
self.storage: Union[DbmCacheStorage, FilesystemCacheStorage] = load_object(settings['HTTPCACHE_STORAGE'])(settings)
Copy link
Contributor

@wRAR wRAR Dec 18, 2019

Choose a reason for hiding this comment

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

These can be any other (user-defined) classes, right?

Copy link
Contributor Author

@grammy-jiang grammy-jiang Dec 18, 2019

Choose a reason for hiding this comment

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

Yes, and now we have only these classes (DummpyPolicy, RFC2616Policy, DbmCacheStorage, FilesystemCacheStorage) in this project.

@wRAR
Copy link
Contributor

wRAR commented Dec 18, 2019

Tests failed because of "SyntaxError: future feature annotations is not defined" on 3.6 and "SyntaxError: invalid syntax" at DOWNLOAD_EXCEPTIONS: Tuple = (defer.TimeoutError, TimeoutError, DNSLookupError on 3.5.

@grammy-jiang
Copy link
Contributor Author

grammy-jiang commented Dec 18, 2019

Tests failed because of "SyntaxError: future feature annotations is not defined" on 3.6 and "SyntaxError: invalid syntax" at DOWNLOAD_EXCEPTIONS: Tuple = (defer.TimeoutError, TimeoutError, DNSLookupError on 3.5.

Yes. For the classmethod from_crawler the typing hint of the return is the class itself. This kind of typing hint itself is only allowed with __future__.annotations, which is added since Python 3.7:

This commit fixes the subclass typing hint with TypeVar
@codecov
Copy link

codecov bot commented Dec 18, 2019

Codecov Report

Merging #4243 into master will increase coverage by 0.01%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master    #4243      +/-   ##
==========================================
+ Coverage   84.81%   84.82%   +0.01%     
==========================================
  Files         163      163              
  Lines        9987     9997      +10     
  Branches     1487     1487              
==========================================
+ Hits         8470     8480      +10     
  Misses       1254     1254              
  Partials      263      263              
Impacted Files Coverage Δ
scrapy/downloadermiddlewares/httpcache.py 93.97% <95.45%> (+0.82%) ⬆️

Copy link
Member

@elacuesta elacuesta left a comment

If I'm not mistaken, you should be able to work around the classmethod issue by supplying the name of the class, i.e.: def from_crawler(cls, crawler: Crawler) -> "HttpCacheMiddleware"

Also, I don't think we are (yet) in a position to support variable annotations, we still support Python 3.5 (#4041 (comment))

scrapy/downloadermiddlewares/httpcache.py Show resolved Hide resolved
@grammy-jiang
Copy link
Contributor Author

grammy-jiang commented Jan 3, 2020

If I'm not mistaken, you should be able to work around the classmethod issue by supplying the name of the class, i.e.: def from_crawler(cls, crawler: Crawler) -> "HttpCacheMiddleware"

Also, I don't think we are (yet) in a position to support variable annotations, we still support Python 3.5 (#4041 (comment))

Yes, in Python 3.5 the variable annotation is not supported and I have removed the typing hint for variables.

Adding typing hints is not a simple work as I expected before. From this PR we have some valuable discussion and we can see there are still some certain misunderstand for the typing module in Python 3.5+. Practicing more will be much helpful.

@grammy-jiang
Copy link
Contributor Author

grammy-jiang commented Jan 8, 2020

Since some of the members of this project have approved this commit, I think maybe we can write a guidelines about how to add the typing hint for Scrapy?

@Gallaecio
Copy link
Member

Gallaecio commented Jan 8, 2020

I guess we can extend the contributing page with such guidelines. What do you have in mind?

@elacuesta
Copy link
Member

elacuesta commented Jan 8, 2020

Sounds good. We should also think about adding a typing check step to the tox/travis configurations.

@@ -33,19 +40,19 @@ def __init__(self, settings, stats):
self.stats = stats

@classmethod
def from_crawler(cls, crawler):
def from_crawler(cls, crawler: Crawler) -> "HttpCacheMiddleware":
Copy link
Member

@kmike kmike Jan 8, 2020

Choose a reason for hiding this comment

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

this method doesn't return HttpCacheMiddleware in a subclass, see https://stackoverflow.com/questions/44640479/mypy-annotation-for-classmethod-returning-instance for a discussion

Copy link
Contributor Author

@grammy-jiang grammy-jiang Jan 10, 2020

Choose a reason for hiding this comment

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

Em, I can see this will be a hot topic in this typing hint topic.

First, should all kinds of possible typing hint be covered? Using typing.Type to cover all subclasses is not a problem. The problem is should we use typing.Type for all classes in Scrapy, just like the previous Request and Response typing hint in this pull request?

Second, annotating the class itself is supported until python 3.7 with the help of __future__.annotations. Since Scrapy is still compatible with python 3.5, what should we do for classmethod return at this moment? A string typing hint is not a good solution in my opinion.

@kmike
Copy link
Member

kmike commented Jan 8, 2020

+1 to add automatic type checks on CI; I think this should be done even before merging any type annotations.

@grammy-jiang
Copy link
Contributor Author

grammy-jiang commented Jan 10, 2020

I guess we can extend the contributing page with such guidelines. What do you have in mind?

From the discussion in this pull request, I can see the following points should be included in this document:

  • The place to add typing hint at the first stage:
    To be compatible with python 3.5, only add typing hint for input and output, not for variables; for classmethod should we add typing hint for return, and in which way, a string like HttpcacheMiddleware? or leave it until in python 3.7 with the power of __future__.annotations to annotate itself?
  • The class of typing hint to mention:
    Should the typing hint include the subclass (we already have two comments about this topic in this pull request and I can imagine there will be more discussion in the community in the future)? Should the typing hint mention all possible classes (with typing.Union)?
  • Some special cases, e.g. the typing hint for a decorator is a nightmare because what kind of function will be decorated in the user's development is very hard to predict.

There are many things uncertain about typing hint. But at least we can start from some easy and clear places now, and keep discussing at the same time until we find a good solution.

Part of code is typing hinted is better than nothing. - Alan Turing

@grammy-jiang
Copy link
Contributor Author

grammy-jiang commented Jan 10, 2020

+1 to add automatic type checks on CI; I think this should be done even before merging any type annotations.

Yes, this will be much helpful for the contributors to test their code before creating the pull request.

Fixing the bugs before merging is much easier than after. - John von Neumann

@elacuesta elacuesta closed this Apr 3, 2020
@elacuesta elacuesta reopened this Apr 3, 2020
@elacuesta
Copy link
Member

elacuesta commented Apr 3, 2020

Typing build added @kmike 🚀

mypy.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
scrapy/__init__.py Outdated Show resolved Hide resolved
@elacuesta
Copy link
Member

elacuesta commented May 22, 2020

Tests are failing because typing.Type was introduced in Python 3.5.2, and we're testing with 3.5.1 since #4518. According to the release schedule page, 3.5.2 was released on June 26th 2016, six months after 3.5.1. It should be safe to bump the requirement IMHO.

@Gallaecio Gallaecio merged commit 55edf8d into scrapy:master Aug 17, 2020
2 checks passed
@Gallaecio
Copy link
Member

Gallaecio commented Aug 17, 2020

Thank you!

@grammy-jiang grammy-jiang deleted the typing-hint branch Aug 30, 2020
joaquingx added a commit to joaquingx/scrapy that referenced this pull request Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants