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

python-dateutil: respect inheritance on relativedelta.__rsub__ (#11462) #11463

Merged
merged 6 commits into from
Mar 15, 2024

Conversation

MaicoTimmerman
Copy link
Contributor

@MaicoTimmerman MaicoTimmerman commented Feb 23, 2024

Support inheritance for relativedelta.__rsub__ by setting the TypeVar upper bound to datetime.date (parent of datetime.datetime), instead of having both those as constraint.

Resolved #11462

This comment has been minimized.

@srittau

This comment was marked as outdated.

@srittau srittau added the status: deferred Issue or PR deferred until some precondition is fixed label Feb 26, 2024
@srittau

This comment was marked as outdated.

@srittau
Copy link
Collaborator

srittau commented Feb 26, 2024

Please ignore my comments about date.__sub__. The stubs, the C, and Python implementation all override it in datetime.__sub__. Our stubs shouldn't match:

class datetime(date):
    # ...
    @overload  # type: ignore[override]
    def __sub__(self, __value: Self) -> timedelta: ...
    @overload
    def __sub__(self, __value: timedelta) -> Self: ...

@MaicoTimmerman
Copy link
Contributor Author

MaicoTimmerman commented Feb 26, 2024

The reason the tests fail is that date.sub has an overload def sub(self, __value: Self) -> timedelta: ..., which of course matches the tested line and therefore returns a timedelta.

__value: Self would match if the RHS is a (subclass of) date, right? But in this case it's dateutil.relativedelta.relativedelta(days=1), which doesn't match. So I think there is some other reason here.

Oddly enough, I wasn't able to reproduce the issue locally using pyright==1.1.342 yet.

edit: just saw your newly added comment.

@srittau srittau removed the status: deferred Issue or PR deferred until some precondition is fixed label Feb 26, 2024
@srittau
Copy link
Collaborator

srittau commented Feb 26, 2024

I'm not sure what's going on. Maybe @erictraut has an idea as it seems that only pyright fails while mypy accepts the tests.

@erictraut
Copy link
Contributor

I'm not able to repro this issue with the latest version of pyright (1.1.351). To try to repro, I manually applied the change in relativedelta.pyi and then verified that the test code in this PR passes without any type errors.

I also tried to repro it with 1.1.342, which is the version of pyright that typeshed is currently pinned to. So I'm not sure what's happening here.

@srittau srittau closed this Feb 26, 2024
@srittau srittau reopened this Feb 26, 2024

This comment has been minimized.

@MaicoTimmerman
Copy link
Contributor Author

@srittau anything I can do to push this forward? The failures persist with the re-run. It almost seems like the test is running without the updated stubs? That was exactly the case I'm trying to fix in this MR.

This comment has been minimized.

@srittau srittau closed this Mar 15, 2024
@srittau srittau reopened this Mar 15, 2024

This comment has been minimized.

@srittau
Copy link
Collaborator

srittau commented Mar 15, 2024

I'm still stumped what the problem here is. I have the same results as @erictraut when testing this PR locally. It really behaves as if the changes to the python-dateutil stubs were ignored.

@srittau
Copy link
Collaborator

srittau commented Mar 15, 2024

Here's what I'm going to do: Comment out the offending line in the test cases and merge this PR if the tests pass. Then I'm going to open another PR restoring the line in the tests. If these pass, it will confirm the suspicion that the current stubs are not picked up.

This comment has been minimized.

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@srittau srittau merged commit bd33ada into python:main Mar 15, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relativedelta __rsub__ doesn't respect subclasses of datetime
4 participants