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

CacheRegion.configure should return typing_extensions.Self #240

Closed
Daverball opened this issue May 23, 2023 · 6 comments
Closed

CacheRegion.configure should return typing_extensions.Self #240

Daverball opened this issue May 23, 2023 · 6 comments
Labels
bug Something isn't working pep484

Comments

@Daverball
Copy link
Contributor

) -> "CacheRegion":

Returning a fixed type here is unfriendly to subclassing CacheRegion to e.g. add new methods.

@zzzeek
Copy link
Member

zzzeek commented May 23, 2023

sure, you can see the blame on there is three years ago before pep-673 was even posted. send a PR

@Daverball
Copy link
Contributor Author

Before that it would've still been possible using def method(self: _T) -> _T: But I agree that it is a little bit more cumbersome to use a TypeVar.

@zzzeek
Copy link
Member

zzzeek commented May 23, 2023

to clarify, pep-673 provides the Self type. this code should be changed as follows:

diff --git a/dogpile/cache/region.py b/dogpile/cache/region.py
index 79f7eaa..952f245 100644
--- a/dogpile/cache/region.py
+++ b/dogpile/cache/region.py
@@ -18,6 +18,7 @@ from typing import Sequence
 from typing import Tuple
 from typing import Type
 from typing import Union
+from typing_extensions import Self
 
 from decorator import decorate
 
@@ -426,7 +427,7 @@ class CacheRegion:
         wrap: Sequence[Union[ProxyBackend, Type[ProxyBackend]]] = (),
         replace_existing_backend: bool = False,
         region_invalidator: Optional[RegionInvalidationStrategy] = None,
-    ) -> "CacheRegion":
+    ) -> Self:
         """Configure a :class:`.CacheRegion`.
 
         The :class:`.CacheRegion` itself
diff --git a/setup.cfg b/setup.cfg
index 319c4b9..3efb2ce 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -24,6 +24,7 @@ project_urls =
 install_requires =
   decorator>=4.0.0
   stevedore>=3.0.0
+  typing_extensions
 zip_safe = False
 packages = find:
 python_requires = >=3.6

@zzzeek zzzeek added bug Something isn't working pep484 labels May 23, 2023
@zzzeek
Copy link
Member

zzzeek commented May 23, 2023

oh, yes, we used to use TypeVar for this kind of thing but that didn't become apparent to me until I had 100's of such methods to do in SQLAlchemy, and even then it had some rough edges

@Viicos Viicos mentioned this issue Jul 8, 2023
@sqla-tester
Copy link
Collaborator

Viicos has proposed a fix for this issue in the main branch:

Use typing.Self https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/4739

1 similar comment
@sqla-tester
Copy link
Collaborator

Viicos has proposed a fix for this issue in the main branch:

Use typing.Self https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/4739

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pep484
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants