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

Type annotations #62

Merged
merged 10 commits into from
Mar 10, 2022
Merged

Type annotations #62

merged 10 commits into from
Mar 10, 2022

Conversation

crusaderky
Copy link
Collaborator

@crusaderky crusaderky commented Mar 5, 2022

name = zict
version = 2.2.0.dev1
maintainer=Matthew Rocklin
maintainer_email=mrocklin@coiled.io
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we change this to a mailing list of the coiled OSS team?

Copy link

Choose a reason for hiding this comment

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

I think that'd be a good idea, but shouldn't hold up the PR.

@crusaderky crusaderky marked this pull request as ready for review March 5, 2022 14:50
@crusaderky crusaderky self-assigned this Mar 5, 2022
crusaderky added a commit to crusaderky/zict that referenced this pull request Mar 7, 2022
@fjetter fjetter requested a review from phobson March 8, 2022 17:25
name = zict
version = 2.2.0.dev1
maintainer=Matthew Rocklin
maintainer_email=mrocklin@coiled.io
Copy link

Choose a reason for hiding this comment

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

I think that'd be a good idea, but shouldn't hold up the PR.

Operating System :: OS Independent
Programming Language :: Python
Programming Language :: Python :: 3
Programming Language :: Python :: 3.7
Copy link

Choose a reason for hiding this comment

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

Any harm in dropping 3.7 here? that would let you implement some of the TODOs in the comments that don't work on 3.7

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's just two small bits; I didn't think it was worth the effort of gathering consensus.

from collections import defaultdict
from collections.abc import Callable, Iterable, Iterator, Mapping, MutableMapping
Copy link

Choose a reason for hiding this comment

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

in common.py and zip.py, MutableMapping is imported from the typing module for compatibility with python <3.9.

But I think MutableMapping has been in python for a while now:
https://docs.python.org/3.7/library/collections.abc.html#collections.abc.MutableMapping

Do we rely on a specific implementation that's only available in > Python3.8?

Copy link
Collaborator Author

@crusaderky crusaderky Mar 10, 2022

Choose a reason for hiding this comment

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

In Python 3.9+, collections.abc.MutableMapping introduces support for square brackets like typing.MutableMapping does.
In other places we're importing from collections.abc; that's because those are modules where MutableMapping[...] is used exclusively in delayed type annotations, which are not parsed by the python interpreter.
In this module however MutableMapping[...] is used as a base class, which means at runtime.

import zipfile
from collections.abc import Iterator
from typing import MutableMapping # TODO move to collections.abc (needs Python >=3.9)
Copy link

Choose a reason for hiding this comment

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

zict/file.py Outdated Show resolved Hide resolved
load: Callable[[WT], VT],
d: MutableMapping[KT, WT],
):
# FIXME https://github.com/python/mypy/issues/708

Choose a reason for hiding this comment

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

Wow, that issue is quite a ride. This hack is particularly fun, though probably a bad idea.

zict/zip.py Outdated Show resolved Hide resolved
@crusaderky crusaderky merged commit 6850845 into dask:main Mar 10, 2022
@crusaderky crusaderky deleted the annotations branch March 10, 2022 17:09
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.

3 participants