Add mild typing to common utils functions#8848
Conversation
I ran into this for parse_timedelta elsewhere. These functions often change types of things and are commonly used. I don't want to get into deep typing here (I'll leave that for others) but a little bit here seemed like it might have unambiguously positive impact.
|
|
||
|
|
||
| def parse_timedelta(s, default="seconds"): | ||
| def parse_timedelta(s, default="seconds") -> float: |
There was a problem hiding this comment.
| def parse_timedelta(s, default="seconds") -> float: | |
| def parse_timedelta(s: float | timedelta | str | None, default="seconds") -> float | None: |
I'm not sure why there is a None code path here, it doesn't look very useful to me, and type checkers should rightfully complain that this function might return None when people try to us the result as a float.
Ideally we would (a) remove the None option, or (b) if it is important somewhere, include an overload:
@overload
def parse_timedelta(s: None) -> None
...
@overload
def parse_timedelta(s: str | float | timedelta) -> float
....
def parse_timedelta(s: str | float | timedelta | None) -> float | None
# implementation|
Look folks, I'm just doing the minimal thing here. I don't want this to distract people. This has already cost enough time that I regret the PR. I'm going to put just enough work in so that this isn't wrong and that's it. I'm not focusing on this right now. I'm focusing on getting shuffling in a good state. I apologize for starting this PR. |
|
If this is good, awesome, let's merge. If there is something broken / damaging then I'll fix it. Let's raise an issue to improve this module in the future (I think that it's one of the higher impact modules for typing) and then prioritize it accordingly. |
|
Looking back over my PRs I saw this. I'm abandoning this now. If someone wants to pick it up that would be grand. I think that it's good to go, but I'll let others make that determination. Happy to close otherwise. |
Co-authored-by: Ian Rose <ian.r.rose@gmail.com>
|
I'm trying to push to this PR but I get permissions error. @mrocklin is the "Allow edits and access to secrets by maintainers" checkbox ticked? |
|
Can't push, will open a new PR |
I ran into this for parse_timedelta elsewhere.
These functions often change types of things and are commonly used.
I don't want to get into deep typing here (I'll leave that for others)
but a little bit here seemed like it might have unambiguously positive
impact.