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

Typing updates #3928

Closed
wants to merge 10 commits into from
Closed

Typing updates #3928

wants to merge 10 commits into from

Conversation

AMDmi3
Copy link
Contributor

@AMDmi3 AMDmi3 commented Jul 22, 2019

What do these changes do?

Bump mypy version to 0.720 and fix/silence new mypy issues.

Are there changes in behavior for the user?

No

Related issue number

#3927

Checklist

  • I think the code is well written
  • Unit tests for the changes exist (no new code or behavior change, should be covered with existing tests)
  • 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 (may do after more changes are merged)
    • 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."

@@ -178,6 +178,8 @@ class ClientSession:
'_request_class', '_response_class',
'_ws_response_class', '_trace_configs')

_default_headers: CIMultiDict[str]
Copy link
Member

Choose a reason for hiding this comment

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

aiohttp still supports Python 3.5, this syntax doesn't work.

I'm open for discussing the drop of Python 3.5 in aiohttp 4.0 though.
If you want to proceed in this direction please create a new issue for discussion.

Otherwise please use Python 3.5 compatible syntax in the PR

@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Jul 23, 2019

Sorry, I'm not really interested in legacy support or participating in related discussions. If this cannot be merged right away, it should either hang until it can be merged, or have only compatible bits cherry picked, or just be discarded.

@webknjaz
Copy link
Member

@webknjaz
Copy link
Member

Curious what Towncrier has to do with this... https://travis-ci.com/aio-libs/aiohttp/jobs/218370235#L527

@asvetlov
Copy link
Member

@webknjaz I don't know but towncrier wants to import the served package for some reason.
Maybe for the package version extraction?

@webknjaz
Copy link
Member

Ah right.. Package version. Makes sense. AFAIR you can use --version if you have an external mechanism of extracting it.
But the thing which makes me confused is that it barks at the syntax error while running in the Python 3.7 env.

@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Aug 1, 2019

it fails even under Python 3.7

Actually I think there's problem with typing support in multidict module which causes this. Not sure how to fix that properly.

% cat 1.py
from multidict import CIMultiDict
foo: CIMultiDict[str] = CIMultiDict()
% mypy --strict 1.py
% python3 1.py
Traceback (most recent call last):
  File "1.py", line 2, in <module>
    foo: CIMultiDict[str] = CIMultiDict()
TypeError: 'type' object is not subscriptable
% cat 2.py
from multidict import CIMultiDict
foo: CIMultiDict = CIMultiDict()
% mypy --strict 2.py
2.py:2: error: Missing type parameters for generic type
% python3 2.py 

@asvetlov
Copy link
Member

asvetlov commented Aug 2, 2019

Use forwarded type declaration (string) :

foo: 'CIMultiDict[str]' = CIMultiDict()

It is not multidict error, asyncio.Future[int] has the same limitation.
Python 3.7 has from __future__ import annotations (see https://www.python.org/dev/peps/pep-0563/ for details).
Since the feature is the mainstream I don't think that multidict should fix the issue itself by providing __getitem__ in own metaclass.

@antonagestam
Copy link

antonagestam commented Aug 5, 2019

@asvetlov Isn't the issue that the C implementation of CIMultiDict lacks a __class_getitem__ method? If I import it using from multidict._multidict_py import CIMultiDict, using CIMultiDict[str] does not trigger an error, since the Python implementation inherits from the abstract base class.

See MagicStack/immutables#13 and https://gitter.im/python/typing?at=5d25bd59cdb70805c96e239f for a similar issue.

@asvetlov
Copy link
Member

asvetlov commented Aug 5, 2019

__class_getitem__ works for Python 3.7+ only, aiohttp is supposed to support older python versions

@antonagestam
Copy link

@asvetlov Yes, but currently using CIMultiDict[str] will break at runtime. The only way around that is to make the C implementation subscriptable. Doing that doesn't break support for older Python versions, but enables the new annotation syntax for users of Python 3.5+.

@asvetlov
Copy link
Member

asvetlov commented Aug 5, 2019

The way is using forward declarations: "CIMultiDict[str]"

@antonagestam
Copy link

What do you mean with "the way"? In my opinion, a better way for the multidict project would be to implement __class_getitem__ and letting the user choose whether to use the string syntax or not? But really this discussion ought to happen in the multidict repo.

@asvetlov
Copy link
Member

asvetlov commented Aug 5, 2019

But really this discussion ought to happen in the multidict repo.

Exactly. aiohttp should use strings here.

- Add type annotations to global variables
- Fix handling of unused return values from time.gmtime
- Silence mypy error on return line (it's not yet capable of deducing
  that _cached_formatted_datetime is always defined at the end of the
  function)
(MatchObject.lastindex may be None)
...by moving field type annotation to class definition
...by moving field type annotation to class definition
...by moving field type annotation to class definition
    error: Incompatible types in "await" (actual type "Generator[Any, None, AbstractServer]", expected type "Awaitable[Any]")

Not sure how to fix, may be a typeshed problem
Not sure how to fix this properly, the logic is hard to follow
@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Aug 13, 2019

Switched to string annotations, python 3.6+ build seem to be fixed

@mjpieters mjpieters mentioned this pull request Aug 30, 2019
5 tasks
@asvetlov
Copy link
Member

Outdated, the master supports newer mypy without errors

@asvetlov asvetlov closed this Oct 18, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants