Add typings for Payload #3294

kornicameister · 2018-09-26T21:38:24Z

What do these changes do?

Adds typing for the aiohttp/payload.py

Are there changes in behavior for the user?

No, these are just hints for the type checkers.

Related issue number

#1749

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

kornicameister · 2018-09-26T21:40:15Z

aiohttp/payload.py

@@ -107,27 +126,27 @@ def __init__(self, value, *, headers=None, content_type=sentinel,
        self._content_type = content_type

    @property
-    def size(self):
+    def size(self) -> Optional[float]:


All those Optional seem a little awkward, but honestly speaking that's how I read the logic. Might as well be that I got that wrong.

It is a pretty common case.
For example AsyncIterablePayload has no size but sends a payload chunk-by-chunk

kornicameister

@asvetlov this is very first attempt to do that. But I guess I am lacking some knowledge here, so any suggestion appreciated.

Firstly, let's maybe concentrate on:

aiohttp/payload.py:73: error: Incompatible default for argument "_CHAIN" (default has type "Type[chain[Any]]", argument has type "Callable[..., Tuple[Type[Payload], Any]]")
aiohttp/payload.py:167: error: Unsupported target for indexed assignment
aiohttp/payload.py:191: error: Too many arguments for "__init__" of "Payload"
aiohttp/payload.py:260: error: Too many arguments for "__init__" of "Payload"
aiohttp/payload.py:381: error: Too many arguments for "__init__" of "Payload"

I will try to dig up on my own regarding too many arguments. But I am all puzzled about the first error in the list. Have no idea how am I suppose to make that passing :(

Also, not sure if the typings all all correct for the payloads that have IO in the name.

And yeah, sorry that it goes like that. Just trying to play open cards ;-)

asvetlov · 2018-09-26T21:57:17Z

Thanks for the PR!
Will find a time for review tomorrow.

aiohttp/payload.py

asvetlov · 2018-09-27T20:33:12Z

aiohttp/payload.py:191: error: Too many arguments for "init" of "Payload"

Since Payload accepts the only value positional argument and the value is already passed in super().__init__() the *args should be dropped from BytesPayload constructor.
Perhaps the same for other similar errors.

asvetlov · 2018-09-27T20:45:55Z

aiohttp/payload.py

-    def __init__(self, value, *, headers=None, content_type=sentinel,
-                 filename=None, encoding=None, **kwargs):
+    _size = None  # type: Optional[float]
+    _headers = None  # type: Optional[LooseHeaders]


_headers has the concrete type Optional[CIMultiDict[str]].

LooseHeaders is used for passing any mapping-like type.
aiohttp client API converts LooseHeaders into CIMultiDict internally.

I'm not 100% sure but a payload should accept CIMultiDict objects only.

@asvetlov Given this documentation, I think that it won't hurt to write typing for __init__ to reflect all the possible types CIMultiDict can accept. I will push the next commit with that and you can check it out later on.

asvetlov · 2018-09-27T20:48:23Z

aiohttp/payload.py

+    def get(self,
+            data: Any,
+            *args: Any,
+            _CHAIN: Callable[..., Tuple[Type['Payload'], Any]]=chain,


Use _CHAIN: Any here, it's ok to shut up the type checker for the argument.

kornicameister · 2018-09-28T20:03:43Z

Ok, so mypy no longer complains, but I am getting TypeError: __init__() got an unexpected keyword argument 'disposition' all over the test execution :/. Wonder what I've changed to cause that. I'd assume that adding types shouldn't have nothing to do with changing the actual functionality thus failing tests.

NVM, found that. Removed one extra **kwargs elsewhere.

codecov-io · 2018-09-28T22:52:57Z

Codecov Report

Merging #3294 into master will decrease coverage by <.01%.
The diff coverage is 98.18%.

@@            Coverage Diff             @@
##           master    #3294      +/-   ##
==========================================
- Coverage   98.03%   98.02%   -0.01%     
==========================================
  Files          43       43              
  Lines        8001     8018      +17     
  Branches     1354     1355       +1     
==========================================
+ Hits         7844     7860      +16     
  Misses         65       65              
- Partials       92       93       +1

Impacted Files	Coverage Δ
aiohttp/typedefs.py	`100% <100%> (ø)`	⬆️
aiohttp/payload.py	`98.59% <98.14%> (-0.39%)`	⬇️

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 da75122...0b234c0. Read the comment docs.

asvetlov · 2018-09-29T18:53:08Z

aiohttp/typedefs.py

-JSONEncoder = Callable[[Any], str]
-JSONDecoder = Callable[[str], Any]
+JSONObj = Dict[str, Any]
+JSON = Union[


Technically JSON can contain a single integer, None or string.
Moreover, it can be any python object if there is python -> JSON converter passed by json.dums(obj, defaul=my_converter).
Let's use Any for json encoders/decoders.

kornicameister · 2018-09-29T21:01:18Z

aiohttp/payload.py

-        super().__init__(
-            value.encode(encoding),
-            encoding=encoding, content_type=content_type, *args, **kwargs)
+        if encoding is None:


Ok, this bit I completely do not get. It's literally impossible for encoding to be None, based on the logic. But the typing says otherwise and we can't exactly redefine the value with mypy. We can't also do the

_encoding: str _content_type: str

letting the assignment to come later, because tests on 3.5 complains. We could've do the typing.cast but not really sure if that's correct. Sounds a bit hacky to me.

@asvetlov any suggestions ?

Alternatively, we can keep that raise ValueError and add some noqa there, but honestly that sounds bad as well to me.

Oh, and for the sake of discussion, this is the alternative:

class StringPayload(BytesPayload): def __init__(self, value: str, *args: Any, encoding: Optional[str]=None, content_type: Optional[str]=None, **kwargs: Any) -> None: _encoding, _content_type = self._parse_args(encoding, content_type) super().__init__( value.encode(_encoding), encoding=_encoding, content_type=_content_type, *args, **kwargs) @staticmethod def _parse_args( encoding: Optional[str]=None, content_type: Optional[str]=None, ) -> Tuple[str, str]: if encoding and content_type: return encoding, content_type elif encoding is None and content_type is None: return 'utf-8', 'text/plain; charset=utf-8' elif encoding is None and content_type: mimetype = parse_mimetype(content_type) return ( mimetype.parameters.get('charset', 'utf-8'), content_type, ) else: return ( cast(str, encoding), 'text/plain; charset=%s' % cast(str, encoding), )

Revert back the change, drop if encoding is None check.
Use the following trick:

if encoding is None: if content_type is None: real_encoding = 'utf-8' content_type = 'text/plain; charset=utf-8' else: mimetype = parse_mimetype(content_type) readl_encoding = mimetype.parameters.get('charset', 'utf-8') else: if content_type is None: content_type = 'text/plain; charset=%s' % encoding real_encoding = encoding super().__init__( value.encode(encoding), encoding=real_encoding, content_type=content_type, *args, **kwargs)

real_encoding variable has strict string type, not Optional[str].

I believe mypy understands it pretty well.

asvetlov · 2018-09-29T21:12:49Z

aiohttp/payload.py


        super().__init__(
            dumps(value).encode(encoding),
            content_type=content_type, encoding=encoding, *args, **kwargs)


+if TYPE_CHECKING:


Please add # pragma: no cover comment to the line for hinting coverage tool

asvetlov · 2018-10-01T10:48:27Z

Please use another approach.
CIMultiDict is a generic class, strict mypy mode forbids unspecialized generics.
CIMultiDict[str] should be used instead.
But it produces a runtime exception, sure.
The easiest backward compatible way to solve it is forward declarations.
Just wrap the problematic annotation in quotes: 'CIMultiDict[str]' and Union[CIMultiDict[str], Dict[str, str]]. The later most likely can be collapsed into Mapping[str, str] though.

kornicameister · 2018-10-01T15:39:27Z

Doesnt it mean that _CIMultiDict should be removed and replaced with forward references ? I mean at all...not as a sugestiom for me. Given your comment I got the feeling I used sth I should not have 😎

asvetlov · 2018-10-01T20:06:59Z

Thanks!
I've missed underscore in _CIMultiDict on review.
I discovered forward-declaration based solution after adding _CIMultiDict to typedefs.py.
Now I have no strong preference, both ways are acceptable.
Let's add type hints everywhere first. Maybe later we'll polish it, I don't care right now.

kornicameister · 2018-10-01T20:11:12Z

Ok, working currently on web_response, but that is a huge beast. I am still trying to figure out all the bits and pieces.

asvetlov · 2018-10-01T20:19:47Z

tracing.py should be a relatively easy thing.

kxepal · 2019-01-04T17:19:58Z

aiohttp/payload.py

-
-    def __init__(self, value, *, headers=None, content_type=sentinel,
-                 filename=None, encoding=None, **kwargs):
+    _size = None  # type: Optional[float]


A bit late for the party, but why size is typed as float? This value is used for Content-Length header which defined number of bytes of payload size - it's hard to pass over HTTP some fraction of a byte. What was the case for float type?

lock · 2020-01-04T20:36:59Z

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.

Add typings for Payload

6835a8d

kornicameister commented Sep 26, 2018

View reviewed changes

asvetlov reviewed Sep 27, 2018

View reviewed changes

aiohttp/payload.py Show resolved Hide resolved

asvetlov reviewed Sep 27, 2018

View reviewed changes

Next iteration for typings

6f06470

kornicameister added 2 commits September 28, 2018 22:34

Add missing kwarg and prevent None encoding

768900e

Python 3.5

9bd36fd

asvetlov requested changes Sep 29, 2018

View reviewed changes

kornicameister commented Sep 29, 2018

View reviewed changes

asvetlov reviewed Sep 29, 2018

View reviewed changes

kornicameister added 3 commits September 30, 2018 00:40

Remove type for JSON and resolve encoding situation

b1c2408

Add missing types for CIMultiDict and make headers str,str

3481c29

Fix towncrier

0b234c0

asvetlov approved these changes Oct 1, 2018

View reviewed changes

asvetlov merged commit 4c4401a into aio-libs:master Oct 1, 2018

kornicameister deleted the add_typings_for_payload branch October 1, 2018 20:10

kxepal reviewed Jan 4, 2019

View reviewed changes

kxepal mentioned this pull request Jan 4, 2019

Change type of Payload.size from Optional[float] to Optional[int] #3484

Merged

lock bot added the outdated label Jan 4, 2020

lock bot locked as resolved and limited conversation to collaborators Jan 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typings for Payload #3294

Add typings for Payload #3294

kornicameister commented Sep 26, 2018 •

edited

Loading

kornicameister Sep 26, 2018 •

edited

Loading

asvetlov Sep 27, 2018

kornicameister left a comment •

edited

Loading

asvetlov commented Sep 26, 2018

asvetlov commented Sep 27, 2018 •

edited

Loading

asvetlov Sep 27, 2018

kornicameister Sep 28, 2018

asvetlov Sep 27, 2018

kornicameister commented Sep 28, 2018 •

edited

Loading

codecov-io commented Sep 28, 2018 •

edited

Loading

asvetlov Sep 29, 2018

kornicameister Sep 29, 2018

kornicameister Sep 29, 2018

kornicameister Sep 29, 2018

asvetlov Sep 29, 2018

asvetlov Sep 29, 2018

asvetlov commented Oct 1, 2018

kornicameister commented Oct 1, 2018 •

edited

Loading

asvetlov commented Oct 1, 2018

kornicameister commented Oct 1, 2018

asvetlov commented Oct 1, 2018

kxepal Jan 4, 2019

kxepal Jan 4, 2019

lock bot commented Jan 4, 2020

Add typings for Payload #3294

Add typings for Payload #3294

Conversation

kornicameister commented Sep 26, 2018 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

kornicameister Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kornicameister left a comment • edited Loading

Choose a reason for hiding this comment

asvetlov commented Sep 26, 2018

asvetlov commented Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kornicameister commented Sep 28, 2018 • edited Loading

codecov-io commented Sep 28, 2018 • edited Loading

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

asvetlov commented Oct 1, 2018

kornicameister commented Oct 1, 2018 • edited Loading

asvetlov commented Oct 1, 2018

kornicameister commented Oct 1, 2018

asvetlov commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lock bot commented Jan 4, 2020

kornicameister commented Sep 26, 2018 •

edited

Loading

kornicameister Sep 26, 2018 •

edited

Loading

kornicameister left a comment •

edited

Loading

asvetlov commented Sep 27, 2018 •

edited

Loading

kornicameister commented Sep 28, 2018 •

edited

Loading

codecov-io commented Sep 28, 2018 •

edited

Loading

kornicameister commented Oct 1, 2018 •

edited

Loading