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

#11753 Improve the type annotation of Deferred.fromFuture #11754

Closed

Conversation

babolivier
Copy link
Contributor

@babolivier babolivier commented Nov 8, 2022

Scope and purpose

Fixes #11753

This PR improves the type annotation for Deferred.fromFuture so that it's inferred from the Future's return type.

Contributor Checklist:

This process applies to all pull requests - no matter how small.
Have a look at our developer documentation before submitting your Pull Request.

Below is a non-exhaustive list (as a reminder):

  • The title of the PR should describe the changes and starts with the associated issue number.
  • A release notes news fragment file was create in src/twisted/newsfragments/ (see: Release notes fragments docs.)
  • The automated tests were updated. this PR is only changing a type annotation so I'm not sure there's any test to update - please advise if this isn't true.
  • Once all checks are green, request a review by leaving a comment that contains exactly the string please review.
    Our bot will trigger the review process, by applying the pending review label
    and requesting a review from the Twisted dev team.

babolivier and others added 3 commits November 8, 2022 16:07
Co-authored-by: Jean-Paul Calderone <exarkun@twistedmatrix.com>
@@ -1041,7 +1041,7 @@ def maybeSucceed(result: object) -> None:
return future

@classmethod
def fromFuture(cls, future: Future) -> "Deferred[Any]":
def fromFuture(cls, future: Future[_T]) -> "Deferred[_T]":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like Future is not generic on Python 3.7, so this fails.

I guess making it a string is the solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess making it a string is the solution?

Probably yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.7 is no longer supported so should be fine now

@babolivier
Copy link
Contributor Author

please review

@chevah-robot chevah-robot requested a review from a team November 9, 2022 11:42
def fromFuture(cls, future: Future) -> "Deferred[Any]":
def fromFuture(
cls, future: "Future[_DeferredResultT]"
) -> "Deferred[_DeferredResultT]":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm very confused about _DeferredResultT being contravariant... :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely wrong, but having spent some time trying to fix this now, it becomes clear that the real signatures for Deferred's methods are fiendishly complex, and this contravariance papers over some of the complexities (by accepting invalid types).

Trying to see if I can golf my way to a successful replacement that doesn't do that.

@glyph
Copy link
Member

glyph commented Nov 22, 2022

@babolivier FYI, #11770 incorporates these commits, and will automatically close this PR as well if it's merged.

@glyph
Copy link
Member

glyph commented Nov 22, 2022

(Not blocking on this one, I'd just like to see a more comprehensive change to the annotations. If someone else comes along and merges it I'll happily integrate the commit.)

@babolivier
Copy link
Contributor Author

Just catching up, I see #11770 has been merged, so I'm closing this one.

@babolivier babolivier closed this Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve type annotation for twisted.internet.defer.Deferred.fromFuture
5 participants