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

Incompatible method overrides #1149

Open
stephenfin opened this issue Nov 29, 2023 · 4 comments
Open

Incompatible method overrides #1149

stephenfin opened this issue Nov 29, 2023 · 4 comments
Assignees
Labels

Comments

@stephenfin
Copy link

Describe the bug

The type hints introduced in 7.3.0 appear to be incomplete and enabling the reportIncompatibleMethodOverride = true setting in pyright highlights this (mypy detects this out-of-the-box). For example:

/tmp/stripe-python/stripe/api_resources/account.py                                                                                                                                                                                                                                                                                                                                                                                          
  /tmp/stripe-python/stripe/api_resources/account.py:3475:9 - error: Method "create" overrides class "CreateableAPIResource" in an incompatible manner                                                                                                                                                                                                                                                                                      
    Parameter "**params" has no corresponding parameter (reportIncompatibleMethodOverride)
  /tmp/stripe-python/stripe/api_resources/account.py:3507:9 - error: Method "_cls_delete" overrides class "DeletableAPIResource" in an incompatible manner
    Parameter "**params" has no corresponding parameter (reportIncompatibleMethodOverride)
  /tmp/stripe-python/stripe/api_resources/account.py:3549:9 - error: Method "delete" overrides class "DeletableAPIResource" in an incompatible manner
    No overload signature in override is compatible with base method (reportIncompatibleMethodOverride)
  /tmp/stripe-python/stripe/api_resources/account.py:3566:9 - error: Method "list" overrides class "ListableAPIResource" in an incompatible manner
    Parameter "**params" has no corresponding parameter (reportIncompatibleMethodOverride)
  /tmp/stripe-python/stripe/api_resources/account.py:3773:9 - error: Method "modify" overrides class "UpdateableAPIResource" in an incompatible manner
    Parameter 2 name mismatch: base parameter is named "sid", override parameter is named "id" (reportIncompatibleMethodOverride)
/tmp/stripe-python/stripe/api_resources/account_link.py
    ...

The root cause of many of these seems to be that the use of typed dict to define parameters for e.g. delete methods makes child methods more restrictive than the parent method (which is an untyped dict), thus violating the Liskov substitution principle. There are other small examples here though.

To Reproduce

Enable reportIncompatibleMethodOverride = true in pyright configuration in pyproject.toml.

Expected behavior

No errors when the reportIncompatibleMethodOverride = true option is enabled in pyright.

Code snippets

No response

OS

Linux

Language version

Python 3.11

Library version

master

API version

n/a

Additional context

No response

@stephenfin stephenfin added the bug label Nov 29, 2023
@richardm-stripe
Copy link
Contributor

richardm-stripe commented Nov 29, 2023

Hello @stephenfin, thanks for the report! Our plan for addressing this is to stop using and deprecate e.g. "ListableAPIResource" in a future major version, although a potential alternative could be to parameterize ListableAPIResource with a generic, e.g. ListableAPIResource[TListParams].

Has this caused problems for you as a user of the library? Please detail if so, this would be pretty troubling.

Enabling more type checking rules and getting stricter types internally is something we are going to work towards long term, but my hope/assumption has been that this wouldn't really have any affect on the external user experience or the types that users see when type-checking their own projects.

@richardm-stripe richardm-stripe self-assigned this Nov 29, 2023
@stephenfin
Copy link
Author

Hi @richardm-stripe. No, no apparent issues with regards to end-use. I spotted this through code inspection while trying to figure out how the new typing all tied together, and I mainly dropped the ticket as an FYI to you folks. Thanks for the thorough response!

@pakrym-stripe
Copy link
Contributor

While we're getting typing issues resolved you can suppress errors by adding

[[tool.mypy.overrides]]
module = "stripe.*"
follow_imports = "silent"

to your pyproject.toml

@richardm-stripe
Copy link
Contributor

Looked with @pakrym-stripe and it turns out that ^ should not be necessary as mypy has a setting --no-silence-site-packages is disabled by default, so type errors do not appear for users of the package unless they have set this setting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants