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

types: make everything type check #540

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Gobot1234
Copy link
Collaborator

@Gobot1234 Gobot1234 commented Oct 28, 2023

Summary

Use more 3.7+ features in the library and make everything type check. This also adds support for using the union operator for python 3.10+ for Union/Optional

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
    • This change has an associated test.
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@Gobot1234 Gobot1234 changed the title Make everything typecheck? Make everything type check Oct 28, 2023
@Gobot1234
Copy link
Collaborator Author

No idea where this pkg_resources failure came from

@cetanu cetanu self-assigned this Oct 29, 2023
cetanu
cetanu previously approved these changes Nov 1, 2023
@Gobot1234
Copy link
Collaborator Author

@cetanu do you have any idea why the tests are failing?

@cetanu
Copy link
Collaborator

cetanu commented Nov 1, 2023

Looking now...

@cetanu
Copy link
Collaborator

cetanu commented Nov 1, 2023

It appears to be related to this

grpc/grpc#33570

but it's just a deprecation warning? I think we can catch this

@cetanu
Copy link
Collaborator

cetanu commented Nov 1, 2023

https://github.com/grpc/grpc/blob/master/tools/distrib/python/grpcio_tools/grpc_tools/protoc.py#L21

@cetanu
Copy link
Collaborator

cetanu commented Nov 1, 2023

Since this is only for generating the tests, we could do something very hacky

    import re
    from importlib import resources

    file = resources.files("grpc_tools") / Path("protoc.py")
    content = file.read_text()

    if "import pkg_resources" in content:
        content = re.sub(
            pattern="import pkg_resources",
            repl=r"try:\n    import pkg_resources\nexcept DeprecationWarning:\n    pass",
            string=content,
        )

    file.open().write(content)

@Gobot1234 Gobot1234 mentioned this pull request Dec 11, 2023
3 tasks
@Gobot1234
Copy link
Collaborator Author

I'll just send a PR to fix it

@tzahush
Copy link

tzahush commented Mar 4, 2024

@Gobot1234 I see the PR that we were waiting for here grpc/grpc#35329 is closed, what should we do as next steps? perhaps this was fixed in different PR's.

@Gobot1234 Gobot1234 changed the title Make everything type check types: make everything type check Mar 24, 2024
@Gobot1234
Copy link
Collaborator Author

Gobot1234 commented Aug 27, 2024

Note to self should try and get more places to use positional only params. Also should probably mark survey methods as abstractmethods(?)

@toby-bro
Copy link

toby-bro commented Sep 12, 2024

mypy still does not manage to type check this correctly, it seems related to this missing feature.
That is supposedly corrected by : https://mypy.readthedocs.io/en/stable/runtime_troubles.html#using-classes-that-are-generic-in-stubs-but-not-at-runtime
I might open a PR for this in the future...
EDIT 1 : I realise you passed to pyright but one of the helpfull functionnalities of betterproto is that it is type hinted and when using it in mypy type checked repos then typing is correct, which is not the case anymore

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.

4 participants