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

Typed library #27

Merged
merged 14 commits into from
Oct 11, 2024
Merged

Typed library #27

merged 14 commits into from
Oct 11, 2024

Conversation

k0t3n
Copy link
Contributor

@k0t3n k0t3n commented Jul 24, 2024

  • Add strict typings for theine lib
  • Deprecate python3.17, add python3.12 tests

* Configure strict mode for mypy
* Add `django-stubs` mypy plugin
@k0t3n
Copy link
Contributor Author

k0t3n commented Jul 24, 2024

@Yiling-J don't you mind making the library fully typed? I submitted an example for the Django adapter, please have a look. If it's ok for you, I can fix the typings for the rest of the codebase.

@Yiling-J
Copy link
Owner

@k0t3n That looks really good! Feel free to add others.

@Yiling-J
Copy link
Owner

Hi @k0t3n I plan to start refactoring/rewriting to support free-threaded in Python 3.13 (#28). Since this might involve a significant refactor/rewrite, I think it would be better to wait your type improvements and releasing a new version for that. Do you have an estimate for when this will be finished?

@k0t3n
Copy link
Contributor Author

k0t3n commented Aug 16, 2024

@Yiling-J I have almost finished with theine lib, but some work still needs to be done on tests/ and benchmarks/, which is tricky in some places. I still wanna finish it up, but unfortunately cannot give you any estimations until September. I see no reasons to lock on my changes if you want to build a new feature.

BWT can we drop support for python 3.7 within these changes? It's not compatible with some stubs and it's EOL finished more than a year ago.

@Yiling-J
Copy link
Owner

@k0t3n I think it's fine to drop 3.7 support. My concern is that if I make significant changes to theine.py, there will be lots conflicts. However I can use your branch as a starting point if you're almost finished with theine lib files.

@k0t3n
Copy link
Contributor Author

k0t3n commented Aug 16, 2024

@Yiling-J please have a look and merge if ok for you. I believe we can work on typings for tests/benchmarks later.

@k0t3n k0t3n marked this pull request as ready for review August 16, 2024 13:59
from theine_core import ClockProCore, LruCore, TlfuCore
from typing_extensions import ParamSpec, Protocol

from theine.exceptions import InvalidTTL
from theine.models import CacheStats

P = ParamSpec("P")
R = TypeVar("R", covariant=True, bound=Any)
R_A = TypeVar("R_A", covariant=True, bound=Union[Awaitable[Any], Callable[..., Any]])
Copy link
Owner

Choose a reason for hiding this comment

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

seems R is replaced by R_A? but R_A bound to Union[Awaitable[Any], Callable[..., Any]], so this code

@Memoize(Cache("tlfu", 1000), None)
def foo(id: int, m: Mock) -> Dict[str, int]:
    m(id)
    return {"id": id}

will not pass mypy check. I already tried and this is the error:

error: Value of type variable "R_A" of "__call__" of "Memoize" cannot be "dict[str, int]"  [type-var]

@Yiling-J
Copy link
Owner

Yiling-J commented Aug 18, 2024

@k0t3n I think it might be better to add a separate test/example file for all public APIs and have mypy check that file in CI. The actual tests/benchmarks don't need to be typed, as they sometimes need to access internal properties, and typing them could make things more complicated.

BTW it seems the Cache class is not typed, do you think it's safe to type it as well?

Yiling-J added a commit that referenced this pull request Aug 26, 2024
@Yiling-J Yiling-J merged commit a5d0419 into Yiling-J:main Oct 11, 2024
5 checks passed
Yiling-J added a commit that referenced this pull request Oct 12, 2024
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.

2 participants