-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add stubs for django-environ #14573
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
base: main
Are you sure you want to change the base?
Add stubs for django-environ #14573
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Thanks! I just want to note that the project doesn’t seem to be very actively maintained (the last commit was 7 months ago), so I wouldn’t expect type hints to be added to the library itself anytime soon. FTR: here’s a recent issue essentially complaining about the lack of type hints: joke2k/django-environ#567, and another one joke2k/django-environ#365 that might give some insight into the maintainer’s stance on adding them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just few moments:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks good overall, see remarks below
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I noticed that you’ve added quite a few |
def __call__( | ||
self, var: _str, cast: type[_dict], default: _dict[_str, _str] | NoValue = ..., parse_default: _bool = False | ||
self, var: _str, cast: type[_dict[Any, Any]], default: _dict[_str, _str] | NoValue = ..., parse_default: _bool = False | ||
) -> _dict[_str, _str]: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that you’ve added quite a few
Any
types in generics. Just a gentle reminder that the use ofAny
is usually accompanied by a short comment explaining why it’s needed. If you’re not entirely sure about the type yet, you might consider usingIncomplete
instead. It could be a more suitable choice in such cases.
@donBarbos You are right. However, what I need is type[dict]
of the builtin function. I think it's awkward, but pyright
requires type arguments in that case. So I added the Any
. Not sure if comments or replace with Incomplete
makes more sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can't specify types just leave Incomplete
, this is a good case.
Anyway in the future it could be improved (type Any
is not very suitable as a placeholder)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced Any
with Incomplete
or object
where applicable. Remaining Any
s are explained with comments.
Diff from mypy_primer, showing the effect of this PR on open source code: discord.py (https://github.com/Rapptz/discord.py)
- discord/ext/commands/hybrid.py:834: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/hybrid.py:834: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
- discord/ext/commands/hybrid.py:858: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/hybrid.py:858: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
- discord/ext/commands/hybrid.py:883: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/hybrid.py:883: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
- discord/ext/commands/hybrid.py:935: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/hybrid.py:935: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
- discord/ext/commands/bot.py:290: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/bot.py:290: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
- discord/ext/commands/bot.py:314: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/bot.py:314: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
|
|
||
# cast dict values | ||
assert_type(env.parse_value("0=TRUE,99=FALSE", {}), dict[str, str]) | ||
assert_type(env.parse_value("0=TRUE,99=FALSE", {"cast": {}}), dict[str, Union[str, object]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the inferred type for this case should ideally be dict[str, str]
. Inferring dict[str, object]
makes this awkward to use for end users, since you can't use the values in the dictionary without casting them or using a type narrowing construct
def cast_person(v: str) -> Person: | ||
parts = v.split(",") | ||
return {"first_name": parts[0], "last_name": parts[1], "age": int(parts[2])} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to include a runtime implementation since these tests aren't executed (we only check how they type check). I think this case is also redundant with custom cast function case above
@@ -0,0 +1,13 @@ | |||
from typing import Final | |||
|
|||
from .compat import DJANGO_POSTGRES as DJANGO_POSTGRES, PYMEMCACHE_DRIVER as PYMEMCACHE_DRIVER, REDIS_DRIVER as REDIS_DRIVER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are indirectly re-exported from .environ
, so let's move the import there
from .compat import DJANGO_POSTGRES as DJANGO_POSTGRES, PYMEMCACHE_DRIVER as PYMEMCACHE_DRIVER, REDIS_DRIVER as REDIS_DRIVER |
from typing_extensions import NotRequired, Required, TypeAlias, Unpack | ||
from urllib.parse import ParseResult | ||
|
||
from .fileaware_mapping import FileAwareMapping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
from .fileaware_mapping import FileAwareMapping | |
from .compat import DJANGO_POSTGRES as DJANGO_POSTGRES, PYMEMCACHE_DRIVER as PYMEMCACHE_DRIVER, REDIS_DRIVER as REDIS_DRIVER | |
from .fileaware_mapping import FileAwareMapping |
_KT = TypeVar("_KT") | ||
_VT = TypeVar("_VT") | ||
|
||
_Cast: TypeAlias = Callable[[str], _T] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd suggest calling this something like _CastFunc
to help distinguish this from the type that's accepted by the various cast
parameters, which is different
|
||
class Path: | ||
def path(self, *paths: str, **kwargs: Unpack[PathKwargs]) -> Path: ... | ||
def file(self, name: str, *args, **kwargs) -> IO[str]: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also return a IO[bytes]
if e.g. mode="rb"
is passed, so let's change this to IO[Any]
.
def file(self, name: str, *args, **kwargs) -> IO[str]: ... | |
# *args and **kwargs are passed to open(). | |
def file(self, name: str, *args: Any, **kwargs: Any) -> IO[Any]: ... |
def rfind(self, s: str, sub: str, start: int = 0, end: int = ...) -> int: ... | ||
def find(self, s: str, sub: str, start: int = 0, end: int = ...) -> int: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def rfind(self, s: str, sub: str, start: int = 0, end: int = ...) -> int: ... | |
def find(self, s: str, sub: str, start: int = 0, end: int = ...) -> int: ... | |
def rfind(self, s: str, start: SupportsIndex | None = ..., end: SupportsIndex | None = ..., /) -> int: ... | |
def find(self, s: str, start: SupportsIndex | None = ..., end: SupportsIndex | None = ..., /) -> int: ... |
|
||
__root__: str | ||
|
||
def __init__(self, start: str = "", *paths: str, **kwargs: Unpack[PathKwargs]) -> None: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
start
and paths
are passed directly to os.path
functions, so any pathlike is accepted.
def __init__(self, start: str = "", *paths: str, **kwargs: Unpack[PathKwargs]) -> None: ... | |
def __init__(self, start: StrPath = "", *paths: StrPath, **kwargs: Unpack[PathKwargs]) -> None: ... |
You'll need to import StrPath
from _typeshed
__root__: str | ||
|
||
def __init__(self, start: str = "", *paths: str, **kwargs: Unpack[PathKwargs]) -> None: ... | ||
def __call__(self, *paths: str, **kwargs: Unpack[PathKwargs]) -> str: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def __call__(self, *paths: str, **kwargs: Unpack[PathKwargs]) -> str: ... | |
def __call__(self, *paths: StrPath, **kwargs: Unpack[PathKwargs]) -> str: ... |
# One CastDict for each combination of 'key', 'value' and 'cast' (8 in total). | ||
# Use auxiliary '_type' to make them distinguishable for type checkers. | ||
@type_check_only | ||
class CastDict000(TypedDict): | ||
_type: NotRequired[Literal["000"]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite follow what's being done with these TypedDicts. It looks like this is a complicated case that would take some effort to get right. In order to not delay getting the rest of the stubs merged, I'd suggest taking a very simple approach for this PR (say, something inferring dict[Any, Any]
for any dict-valued cast
argument) and work on refining the dict cast behavior in a followup
Types for
django-environ
.