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 return typing of AsyncEngine._trans_ctx.__aenter__ #110

Closed

Conversation

FasterSpeeding
Copy link
Contributor

@FasterSpeeding FasterSpeeding commented May 17, 2021

Fixes: #109

Description

In-order to fix AsyncEngine._trans_ctx's typing I have made StartableContext generic rather than assuming it's methods will always be returning a sub-class of itself which isn't the case for _trans_ctx which inherits from it.

As a note, for now I've left the usage of type vars for the return typing of StartableContext.start implementations without using the same type vars when inheriting from StartableContext as this would mark the class as being generic which probably isn't correct. While nothing seems to subclass any of the classes which directly inherit from StartableContext, if they were StartableContext would have to be directly inherited again (only in the stubs) to specialise the behaviour of it further under the current behaviour this proposal introduces e.g:

class Foo(abc.ABC, typing.Generic[_T]):
    @abc.abstractmethod
    def foo(self) -> _T: ...


BarT = typing.TypeVar("BarT", bound="Bar")


class Bar(Foo["Bar"]):
    def foo(self: BarT) -> BarT: ...

class Bat(Bar, Foo["Bat"]):
    def foo(self: BarT) -> BarT: ...

Checklist

This pull request is:

  • A documentation / typographical error fix
    • Good to go, no issue or tests are needed
  • A short code fix
    • please include the issue number, and create an issue if none exists, which
      must include a complete example of the issue. one line code fixes without an
      issue and demonstration will not be accepted.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests. one line code fixes without tests will not be accepted.
  • A new feature implementation
    • please include the issue number, and create an issue if none exists, which must
      include a complete example of how the feature would look.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests.

Have a nice day!

* StartableContext is made generic as to remove the assumption that it'll always be returning itself which is not the case for for it's behaviour in _trans_ctx
@FasterSpeeding
Copy link
Contributor Author

FasterSpeeding commented May 17, 2021

Oh I also noticed an out of place return type hint on the main repo's declaration of StartableContext.start which seems to make this same false assumption but wasn't exactly sure if it'd be worth it doing anything about that
https://github.com/sqlalchemy/sqlalchemy/blob/c379f80b4e6fb1b65983958116bac202c91a210a/lib/sqlalchemy/ext/asyncio/base.py#L8
Seems that's already being removed by another proposed change

@FasterSpeeding
Copy link
Contributor Author

This approach has implications for people who are using these stubs and StartableContext in their typing directly since their type checkers will think it's generic but at runtime it won't be but this is about the best I could come up with solution wise.

Copy link
Contributor

@MaicoTimmerman MaicoTimmerman left a comment

Choose a reason for hiding this comment

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

I believe these changes are good, however I'd like to include the MCVE you've written in #109 as a test-case in this PR as well.

You can place it in test/files/.

@MaicoTimmerman
Copy link
Contributor

I've merged your commit in #128, including the test-case. Thanks for your contribution!

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.

AsyncEngine._trans_ctx typed as returning self on aenter rather than AsyncConn
2 participants