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. |
donbarbos
left a comment
There was a problem hiding this comment.
Looks good! Just few moments:
brianschubert
left a comment
There was a problem hiding this comment.
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.
I noticed that you’ve added quite a few
Anytypes in generics. Just a gentle reminder that the use ofAnyis usually accompanied by a short comment explaining why it’s needed. If you’re not entirely sure about the type yet, you might consider usingIncompleteinstead. 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.
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.
I replaced Any with Incomplete or object where applicable. Remaining Anys are explained with comments.
This comment has been minimized.
This comment has been minimized.
|
|
||
| # 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.
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.
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.
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.
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.
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.
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.
| 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.
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.
| 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.
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
There was a problem hiding this comment.
Yeah, this was quite complicated. I simplified it to just dict[Any, Any] for now.
|
In a lot of my own OPTIONAL_SETTING: str | None = environ.Env().str("DJANGO_OPTIONAL_SETTING", default=None)This is allowed by the library behavior, I'm wondering if the type stubs could also allow something like: _DefaultT = TypeVar("_DefaultT")
class Env:
@typing.overload
def str(self, var: _str, default: _DefaultT = ..., multiline: _bool = False) -> _DefaultT: ...
@typing.overload
def str(self, var: _str, multiline: _bool = False) -> _str: Regarding this idea:
|
|
@EmCeeEs @brianschubert Thanks for all your efforts here! This PR seems to be inactive now, but I'm very interested in seeing it merged. Is it possible for me to provide any help to finish it? |
|
Hi all! I’m actively maintaining this project and I’d be very interested in shipping a 1.0 release with proper typing support. Happy to collaborate to make that happen. |
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
|
I pushed changes addressing @brianschubert's feedback; is this good to go in now? |
Types for
django-environ.