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

fix(python): Overloading date_range #5270

Merged
merged 4 commits into from
Oct 20, 2022

Conversation

thomasaarholt
Copy link
Contributor

@thomasaarholt thomasaarholt commented Oct 19, 2022

EDIT: Now passes, see comments below.

This is my best shot at this.
Mypy still fails, with polars/internals/functions.py:208: error: Overloaded function signatures 3 and 4 overlap with incompatible return types [misc]

These are signatures 3 and 4:

# sig 3
@overload
def date_range(
    low: date | datetime | str,
    high: date | datetime | str,
    interval: str | timedelta,
    closed: ClosedWindow = "both",
    name: str | None = None,
    time_unit: TimeUnit | None = None,
    time_zone: str | None = None,
    lazy: Literal[False] = False,
) -> pli.Series:
    ...

# sig 4
@overload
def date_range(
    low: date | datetime | pli.Expr | str,
    high: date | datetime | pli.Expr | str,
    interval: str | timedelta,
    closed: ClosedWindow = "both",
    name: str | None = None,
    time_unit: TimeUnit | None = None,
    time_zone: str | None = None,
    lazy: Literal[True] = True,
) -> pli.Expr:
    ...

I think they quite explicitly don't overlap though:

# sig 3
@overload
def date_range(
    (...)
    lazy: Literal[False] = False,
) -> pli.Series:
    ...

# sig 4
@overload
def date_range(
    (...)
    lazy: Literal[True] = True,
) -> pli.Expr:
    ...

If I omit the default argument (= True or = False), then mypy passes, but we don't want to omit it.

This to me seems like a bug in mypy.

@thomasaarholt thomasaarholt changed the title best attempt at overload Attempt at overloading date_range Oct 19, 2022
@thomasaarholt thomasaarholt changed the title Attempt at overloading date_range fix(python): Attempt at overloading date_range Oct 19, 2022
@github-actions github-actions bot added fix Bug fix python Related to Python Polars labels Oct 19, 2022
@thomasaarholt
Copy link
Contributor Author

Fixed! I'm assuming that the test failures are unrelated to this PR.

I got one step further by realising that overloaded arguments shouldn't have default args, and then I read this comment on a relevant mypy issue, and based myself on their example.

Unfortunately, this does mean that we have to move the argument to before the default args if you want function overloading.

@thomasaarholt thomasaarholt changed the title fix(python): Attempt at overloading date_range fix(python): Overloading date_range Oct 19, 2022
@ritchie46
Copy link
Member

Thanks a lot. 🙏

@ritchie46 ritchie46 merged commit 9c52dc3 into pola-rs:date_range Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants