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

Please consider fixing type annotations #39

Closed
NeilGirdhar opened this issue Dec 16, 2021 · 23 comments
Closed

Please consider fixing type annotations #39

NeilGirdhar opened this issue Dec 16, 2021 · 23 comments
Labels
Needs: Help Extra attention is needed Needs: Test Hey, it compiles! Ship it! Priority: Low Not a big problem... Type: Enhancement New feature or request

Comments

@NeilGirdhar
Copy link

NeilGirdhar commented Dec 16, 2021

It appears that there are some missing files:

from frozendict import frozendict  # error: Library stubs not installed for "frozendict" (or incompatible with Python 3.10)
reveal_type(dict.fromkeys("abc", 0))  # Revealed type is "builtins.dict[builtins.str*, builtins.int*]"
reveal_type(frozendict.fromkeys("abc", 0))  # Revealed type is "Any"

If you want type annotations to work, you could consider

  • exposing a py.typed file so that static typing is enabled,
  • annotating the methods in core.py, which should be easy now that Python 3.6 is (almost) dead.
@NeilGirdhar NeilGirdhar added the Type: Enhancement New feature or request label Dec 16, 2021
@NeilGirdhar NeilGirdhar changed the title Fix type annotations Please consider fixing type annotations Dec 16, 2021
@Marco-Sulla
Copy link
Owner

I have no knowledge about this. If you want to create a PR, I can consider it :)

@Marco-Sulla Marco-Sulla added Priority: Low Not a big problem... Needs: Help Extra attention is needed labels Dec 22, 2021
@peku33
Copy link

peku33 commented Jan 17, 2022

In the current design the frozendict inherits from dict which is not typed in python 3.8 but typed in python 3.9.
It is not possible to use frozendict[K, V] in 3.8, while the same is valid for 3.9.
Users in 3.8 currently can use typing.Dict approach. Is there similar typed frozendict version compatible with 3.8?

@NeilGirdhar
Copy link
Author

@peku33 Where are you finding frozendict[K, V]?

@peku33
Copy link

peku33 commented Jan 17, 2022

In my code. frozendict[K, V] is correctly recognized by python 3.10, mypy, pylance etc.
When put (for example) inside class / dataclass definition - types are hinted properly.

@NeilGirdhar
Copy link
Author

@peku33 I see because frozendict inherits from dict, and dict defines class_getattr. Well, that would be unaffected by this issue, which merely asks for the additional methods to be annotated.

@Marco-Sulla
Copy link
Owner

In the current design the frozendict inherits from dict

Well, this is true for the pure Py version, not for the C extension, that is the default from version 2.2.0 (the current one).

Users in 3.8 currently can use typing.Dict approach. Is there similar typed frozendict version compatible with 3.8?

I think I can add it. The problem is I really don't know how. I admit I never used type hinting. I have to study.

@peku33
Copy link

peku33 commented Jan 17, 2022

I think for python below 3.9 (which introduces the typed dict) (and possibly higher then 3.6?) the base class should be typings.Dict, not dict.

This can possibly be done with if sys.version_info < (3, 9): or somethign similar.

@NeilGirdhar
Copy link
Author

I think I can add it. The problem is I really don't know how. I admit I never used type hinting. I have to study.

Why would you add it? frozendict is not a dict. If you're going to add something, just inherit from Mapping[K, V] and Generic[K, V]. Then you'll satisfy his annotations without promising all kinds of things that you don't support.

the base class should be typings.Dict, not dict.

No it should not. A frozendict is not a dict.

@peku33
Copy link

peku33 commented Jan 17, 2022

Yes, thing like Mapping is probably fine. I thought the implementation relies on dict underneath and just shadows some methods.

The goal is to have typed declaration in class / function footprint with typed methods etc.

@NeilGirdhar
Copy link
Author

Well, this is true for the pure Py version, not for the C extension, that is the default from version 2.2.0 (the current one).

As far as the annotations go, it's true for both versions I imagine.

@Marco-Sulla
Copy link
Owner

Marco-Sulla commented Jan 17, 2022

Yes, thing like Mapping is probably fine. I thought the implementation relies on dict underneath and just shadows some methods.

This is true for the python implementation, not for the C Extension. frozendict in the C Extension inherits only from Mapping and object, and has some methods optimized for the speed. In some jobs it's faster than dict.

The goal is to have typed declaration in class / function footprint with typed methods etc.

I agree.

@lurch
Copy link
Contributor

lurch commented Aug 14, 2022

I know nothing about mypy and type annotations, but does #62 also fix this issue? 🤷

@Marco-Sulla
Copy link
Owner

I know nothing about mypy and type annotations, but does #62 also fix this issue? shrug

No.

@Marco-Sulla
Copy link
Owner

Currently, annotations can be enabled by merging #70 , but I need to add some tests before and I don't know where to start.

@Marco-Sulla Marco-Sulla added the Needs: Test Hey, it compiles! Ship it! label Feb 25, 2023
@ydirson
Copy link

ydirson commented Feb 25, 2023

With version 2.3.5, as well as when merging #70 (1d81d08) into master (9688014) all uses of the frozendict type trigger a mypy error: Module not callable: looks like something confuses mypy into thinking we're accessing the module, when we're accessing the same-name type from that module.

@Marco-Sulla
Copy link
Owner

@ydirson steps to reproduce?

@ydirson
Copy link

ydirson commented Feb 25, 2023

Testing this code in a brand new venv with latest frozendict and mypy:

from frozendict import frozendict

d = frozendict(a=1, b=2)
print(d)
user@perso-dev:tmp$ python3 -m venv env
user@perso-dev:tmp$ . env/bin/activate
(env) user@perso-dev:tmp$ pip install mypy
Collecting mypy
  Downloading mypy-1.0.1-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (18.5 MB)
     |████████████████████████████████| 18.5 MB 12.2 MB/s 
Collecting typing-extensions>=3.10
  Downloading typing_extensions-4.5.0-py3-none-any.whl (27 kB)
Collecting tomli>=1.1.0
  Using cached tomli-2.0.1-py3-none-any.whl (12 kB)
Collecting mypy-extensions>=0.4.3
  Downloading mypy_extensions-1.0.0-py3-none-any.whl (4.7 kB)
Installing collected packages: typing-extensions, tomli, mypy-extensions, mypy
Successfully installed mypy-1.0.1 mypy-extensions-1.0.0 tomli-2.0.1 typing-extensions-4.5.0
(env) user@perso-dev:tmp$ pip install frozendict
Collecting frozendict
  Using cached frozendict-2.3.5-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (112 kB)
Installing collected packages: frozendict
Successfully installed frozendict-2.3.5
(env) user@perso-dev:tmp$ mypy foo.py 
foo.py:3: error: Module not callable  [operator]
Found 1 error in 1 file (checked 1 source file)

Note that with 2.3.4 the error is as follows, but can be silenced with a simple type: ignore on the import, whereas with 2.3.5 every single use of the class has to be patched.

/home/user/tmp/foo.py:3: error: Module has no attribute "frozendict"  [attr-defined]

@Marco-Sulla
Copy link
Owner

Reproducible with Python 3.9, but not with Python 3.10.

@scottgigante-immunai
Copy link

Here's my workaround (tested in python3.8):

from typing import TypeVar, Generic
from frozendict import FrozenOrderedDict

FrozenDictKeyT = TypeVar("FrozenDictKeyT")
FrozenDictValueT = TypeVar("FrozenDictValueT")

class FrozenDict(Generic[FrozenDictKeyT, FrozenDictValueT], FrozenOrderedDict):
    pass

@Marco-Sulla
Copy link
Owner

@scottgigante-immunai please see #70 , it will be merged in minutes.

@Marco-Sulla
Copy link
Owner

Well, it seems there were big improvements:

https://github.com/Marco-Sulla/python-frozendict/blob/d5b40fc88fd993f1421099bc753b86c57be562de/src/frozendict/__init__.pyi

Unluckily it's not perfect, and you can see it easily with mypy test/typed.py:

  • keyword only constructor is not recognized and key type is lost
  • subclasses of frozendict lose the info about the type of keys and values
  • if frozendict is contructed from a dict or a seq2, mypy signals that __or__ returns a dict (that is not true).

Anyway, if nobody is contrary, I think I'll release it when I have time.

@juliusfrost
Copy link

@Marco-Sulla Thanks for the fix! The release would solve some CI issues I'm having :)

@Marco-Sulla
Copy link
Owner

frozendict 2.3.8 released:

https://github.com/Marco-Sulla/python-frozendict/releases/tag/v2.3.8

I think I can close this issue. For some specific problem about type hints, feel free to open other issues.

PS: what I promise, I'll support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Help Extra attention is needed Needs: Test Hey, it compiles! Ship it! Priority: Low Not a big problem... Type: Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants