Add typings for web_response #3317

kornicameister · 2018-10-03T18:36:15Z

Add typings for aiohttp/web_response.py

For now it is not clear.

Related issue number

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

If there isn't a .netrc file specified by an environment variable, it can be confusing to see warnings about it. If NETRC isn't set, don't warn. Only warn if: a) can't resolve HOME, b) can load, but can't parse file, c) can't find file, d) file appears to exist at the default location but is unreadable for some reason.

kornicameister

@asvetlov the PR isn't finished, but the amount of doubts was the reason for my to ask for some early look at this. Hope you have some time to check it out.

kornicameister · 2018-10-03T18:37:23Z

aiohttp/helpers.py

@@ -484,13 +484,13 @@ def _format_r(request: 'BaseRequest',
    @staticmethod
    def _format_s(request: 'BaseRequest',
                  response: 'StreamResponse',
-                  time: float) -> str:
+                  time: float) -> int:


Correct if I am wrong, but doing typings in web_response.py reveled that it should be int most likely.

You are right.
As an option, we can return str but should apply str(response.status).
Let's change annotation as you did already.

kornicameister · 2018-10-03T18:37:56Z

aiohttp/payload.py

@@ -101,7 +101,7 @@ def register(self,

 class Payload(ABC):

-    _size = None  # type: Optional[float]
+    _size = None  # type: Optional[int]


Yeah, I am pretty sure that making this float in the beginning was an err on my side.

Agree, 3.5 bytes will never happen.

kornicameister · 2018-10-03T18:39:20Z

aiohttp/web_response.py

        if params:
-            ctype = self._content_type + '; ' + params
+            ctype = self.content_type + '; ' + params


Not really sure why variable is Optional[str] and attr getter from super returns str. Can't tell yet which one is correct.

kornicameister · 2018-10-03T18:41:12Z

aiohttp/web_response.py

        return len(self._state)

-    def __iter__(self):
-        return iter(self._state)
+    def __iter__(self) -> Iterator[Tuple[str, str]]:


I will try to look into this once again but basically this one says that aiohttp/web_response.py:483: error: Return type of "__iter__" incompatible with supertype "Iterable" . An Iterable is last or the one before last in the chain of inheritance starting from MutableMapping. I can't really say why this one is bad if typings for MutableMapping says exactly MutableMapping[str, str].

kornicameister · 2018-10-03T18:42:25Z

aiohttp/web_response.py

        if body is None:
            self._body = None
-            self._body_payload = False


So, it turns out that _body can be either bytes or Payload. Seemed more correct to rely on isinstance then keeping extra property in the class.

kornicameister · 2018-10-03T18:42:47Z

aiohttp/web_response.py

-        return self._body.decode(self.charset or 'utf-8')
+        else:
+            if isinstance(self._body, payload.Payload):
+                raise ValueError('Cannot extract text from payload')


Most likely this is wrong :/

asvetlov · 2018-10-03T19:04:41Z

aiohttp/web_response.py

        self._cookies = SimpleCookie()

-        self._req = None
-        self._payload_writer = None
+        self._req = None  # type: Optional[Request]


Optional[BaseRequest] here

asvetlov · 2018-10-03T19:07:29Z

aiohttp/web_response.py

        return self._payload_writer is not None

    @property
-    def task(self):
+    def task(self) -> asyncio.Task[_T]:


asyncio.Task[None]. https://github.com/aio-libs/aiohttp/blob/master/aiohttp/web_protocol.py#L336 returns nothing.

asvetlov · 2018-10-03T19:09:32Z

aiohttp/web_response.py

        return self._compression

    @property
-    def reason(self):
+    def reason(self) -> Optional[str]:


resp.reason is always str (can be an empty string though).

asvetlov · 2018-10-03T19:11:04Z

aiohttp/web_response.py

        return self._body_length

    @property
-    def output_length(self):
+    def output_length(self) -> None:


I guess the length is int

asvetlov · 2018-10-03T19:12:23Z

aiohttp/web_response.py


-    def enable_chunked_encoding(self, chunk_size=None):
+    def enable_chunked_encoding(self, chunk_size: Any=None) -> None:


chunk_size: Optional[int].
The parameter will be removed in aiohttp 4.0 but now let's be consistent.

I figure that since it is not used in anyway and setting it to anything else other than None will anyway result in an error, we actually don't have to care about that. But if you prefer to have it as Optional[int], let it be so.

kornicameister · 2018-10-10T19:14:13Z

@asvetlov sorry for longer inactive. Didn't exactly had time in the evenings to take a look here.

The current status is that I have two errors for which I can't find the solution:

aiohttp/web_response.py:105: error: Cannot determine type of 'RESPONSES'
aiohttp/web_response.py:356: error: Cannot determine type of 'SERVER_SOFTWARE'

Surprisingly that happens only if I use make mypy. Running mypy directly does not report that. I think I saw the fix for it elsewhere I just can't find it, so any help appreciated.

…cktimko-master

* Update async-timeout from 3.0.0 to 3.0.1 * Update cython from 0.28.5 to 0.29 * Update cython from 0.28.5 to 0.29 * Update cython from 0.28.5 to 0.29 * Update gunicorn from 19.8.1 to 19.9.0 * Update tox from 3.5.0 to 3.5.2

…nicameister/aiohttp into kornicameister-add_typings_for_web_response

asvetlov · 2018-10-15T19:59:13Z

I've fixed mypy errors but looks like the logic is broken somehow -- 10 tests are failed.

asvetlov · 2018-10-17T13:32:05Z

Done by 22a58e2

kornicameister · 2018-10-17T20:37:01Z

@asvetlov I was aware that logic will be broken somehow. I couldn't actually get around mypy here and wanted to handle tests after that's done. But if you managed to resolve that, I am more than happy ;-)

kornicameister · 2018-10-17T20:39:04Z

BTW: @asvetlov really nice product. Maybe you recall me mentioning performance comparision (or maybe I've mentioned that over in connexion), but comparing aiohttp with ordinary flask based application, well we, roughly speaking, improved performance about 30%-50%. So really great product 👍

asvetlov · 2018-10-18T07:24:06Z

Thanks for the feedback.
You know, any benchmarking is cheating because hard to compare the same functionality; synthetic benchmark code is very different than your production one.
That's why I love to see any performance comparisons of real applications.

kornicameister · 2018-10-18T18:51:54Z

Heh, it was real application. Although for the sake of POC I've compared just two endpoints (those destined to work under heaviest load). But application was normally working inside the cluster (i.e. it was normally deployed) and tests were ran also in there. So we've been actually sending bunch of requests, sometimes even spawning the service. And what was nearly a heart attack for older app, new one was still catching up.

asvetlov · 2018-10-18T18:55:45Z

Sweet!

lock · 2019-10-28T00:03:55Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

kornicameister commented Oct 3, 2018

View reviewed changes

asvetlov reviewed Oct 3, 2018

View reviewed changes

kornicameister added 5 commits October 10, 2018 18:26

Add typings for web_response

6b1d8f1

Review 1

045bacd

ignore error

c37eedd

Fixed all warnings

e4423e4

Set valid annotations for web_log

cb2f879

asvetlov and others added 6 commits October 13, 2018 02:04

Don't create temporary list in parse_mimetype()

f4c8232

Wrap parse_mimetype in an lru_cache (#3341)

aac7a69

Merge branch 'master' of https://github.com/nicktimko/aiohttp into ni…

7013d4a

…cktimko-master

Merge branch 'nicktimko-master'

c8485f6

Scheduled weekly dependency update for week 41 (#3344)

b26d4a8

* Update async-timeout from 3.0.0 to 3.0.1 * Update cython from 0.28.5 to 0.29 * Update cython from 0.28.5 to 0.29 * Update cython from 0.28.5 to 0.29 * Update gunicorn from 19.8.1 to 19.9.0 * Update tox from 3.5.0 to 3.5.2

Merge branch 'add_typings_for_web_response' of https://github.com/kor…

622d9da

…nicameister/aiohttp into kornicameister-add_typings_for_web_response

Fix mypy

ffc67e0

asvetlov closed this Oct 17, 2018

kornicameister deleted the add_typings_for_web_response branch October 17, 2018 20:35

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typings for web_response #3317

Add typings for web_response #3317

kornicameister commented Oct 3, 2018

kornicameister left a comment

kornicameister Oct 3, 2018

asvetlov Oct 3, 2018

kornicameister Oct 3, 2018

asvetlov Oct 3, 2018

kornicameister Oct 3, 2018

kornicameister Oct 3, 2018

kornicameister Oct 3, 2018

kornicameister Oct 3, 2018

asvetlov Oct 3, 2018

asvetlov Oct 3, 2018

asvetlov Oct 3, 2018

asvetlov Oct 3, 2018

asvetlov Oct 3, 2018

kornicameister Oct 3, 2018

kornicameister commented Oct 10, 2018

asvetlov commented Oct 15, 2018

asvetlov commented Oct 17, 2018

kornicameister commented Oct 17, 2018

kornicameister commented Oct 17, 2018

asvetlov commented Oct 18, 2018

kornicameister commented Oct 18, 2018

asvetlov commented Oct 18, 2018

lock bot commented Oct 28, 2019


		def enable_chunked_encoding(self, chunk_size=None):
		def enable_chunked_encoding(self, chunk_size: Any=None) -> None:

Add typings for web_response #3317

Add typings for web_response #3317

Conversation

kornicameister commented Oct 3, 2018

Related issue number

Checklist

kornicameister left a 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

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

kornicameister commented Oct 10, 2018

asvetlov commented Oct 15, 2018

asvetlov commented Oct 17, 2018

kornicameister commented Oct 17, 2018

kornicameister commented Oct 17, 2018

asvetlov commented Oct 18, 2018

kornicameister commented Oct 18, 2018

asvetlov commented Oct 18, 2018

lock bot commented Oct 28, 2019