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

10231 Fix type annotation for inlineCallbacks #1632

Merged
merged 7 commits into from
Jul 19, 2021

Conversation

richvdh
Copy link
Contributor

@richvdh richvdh commented Jul 18, 2021

Scope and purpose

Propagate the ReturnType of the generators wrapped by inlineCallbacks.

Contributor Checklist:

  • The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/10231
  • I ran tox -e lint to format my patch to meet the Twisted Coding Standard
  • I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
  • The title of the PR starts with the associated Trac ticket number (without the # character).
  • I have updated the automated tests and checked that all checks for the PR are green.
  • I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.
  • The merge commit will use the below format
    The first line is automatically generated by GitHub based on PR ID and branch name.
    The other lines generated by GitHub should be replaced.
Merge pull request #123 from twisted/4356-branch-name-with-trac-id

Author: richvdh
Reviewer: 
Fixes: ticket:10231

Update the type annotation on `inlineCallbacks`.

@adiroiban
Copy link
Member

Thanks for the quick PR.
Can we have it targeting trunk?

I will the cherry pick it in the release branch.

Thanks

@richvdh richvdh changed the base branch from release-21.7.0-10214 to trunk July 18, 2021 23:02
@richvdh
Copy link
Contributor Author

richvdh commented Jul 18, 2021

sure, done

@adiroiban adiroiban requested review from graingert and a team July 18, 2021 23:20
@graingert
Copy link
Member

graingert commented Jul 19, 2021

I'm not really sure what the point of setting an accurate return type annotation on inlineCallbacks is at all. Currently the lack of ParamSpec support means the arguments of the decorated function can't be preserved, and if you can use return value syntax in a generator in your source code then you can use the preferred async/await syntax that supports Deferred annotations fully

@richvdh
Copy link
Contributor Author

richvdh commented Jul 19, 2021

I'm not really sure what the point of setting an accurate return type annotation on inlineCallbacks is at all. Currently the lack of ParamSpec support means the arguments of the decorated function can't be preserved, and if you can use return value syntax in a generator in your source code then you can use the preferred async/await syntax that supports Deferred annotations fully

I don't agree with the last part of this. There are still situations where it is useful to use inlineCallbacks - and have it return a function which returns a Deferred of the right type. Two examples:

  • You need to maintain compatibility with older code which expects a Deferred rather than a coroutine - either because the caller hasn't yet been converted to async/await or because you're preserving a public API.
  • You're implementing an interface. For example, we have an implementation of IAgent, where IAgent.request is defined to return a Deferred - so an async function is not acceptable.

@graingert
Copy link
Member

graingert commented Jul 19, 2021

You can still make a deferred returning function from an async function, and it preserves the call signature:

async def _request(self, ham: int) -> None:
    await x

def request(self, ham: int) -> Deferred[None]:
    return Deferred.fromCoroutine(self._request(ham))

@richvdh
Copy link
Contributor Author

richvdh commented Jul 19, 2021

You can still make a deferred returning function from an async function, and it preserves the call signature:

Well, I can, but the fact I can work around it seems a poor argument against improving the type annotation on inlineCallbacks anyway?

Copy link
Member

@graingert graingert left a comment

Choose a reason for hiding this comment

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

Newsfragment typo

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Thanks you all for the contribution.

The news fragment make sense for me now.

As long as this change is not braking anything else, I think that is OK to merge it.

@graingert any comment against merging it?

Co-authored-by: Thomas Grainger <tagrain@gmail.com>
@graingert
Copy link
Member

closing and re-opening to kick off azure pipelines

@graingert graingert closed this Jul 19, 2021
@graingert graingert reopened this Jul 19, 2021
@graingert graingert merged commit 9756359 into twisted:trunk Jul 19, 2021
@richvdh richvdh changed the title 10214 Fix type annotation for inlineCallbacks 10231 Fix type annotation for inlineCallbacks Jul 22, 2021
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.

None yet

3 participants