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

Handle typing collisions and add validation to a files module for overlaping declarations #582

Merged
merged 8 commits into from
Jul 19, 2024

Conversation

imcdo
Copy link
Contributor

@imcdo imcdo commented Jun 12, 2024

Summary

This PR builds off of the work here, addressing the issue I posted here: #580.

As mentioned by @scott-hendricks we encountered coliding imports when the k8s protos defined their own List class.

Traceback (most recent call last):
...
    from ...k8s.io.api.core import v1 as __k8_s_io_api_core_v1__
  File "/.../k8s/io/api/core/v1/__init__.py", line 16, in <module>
    from ....apimachinery.pkg.apis.meta import v1 as ___apimachinery_pkg_apis_meta_v1__
  File "/.../k8s/io/apimachinery/pkg/apis/meta/v1/__init__.py", line 797, in <module>
    class ObjectMeta(betterproto.Message):
  File "/.../k8s/io/apimachinery/pkg/apis/meta/v1/__init__.py", line 931, in ObjectMeta
    owner_references: List["OwnerReference"] = betterproto.message_field(13)
                      ~~~~^^^^^^^^^^^^^^^^^^
TypeError: type 'List' is not subscriptable

To address this I've added different options to execute the plugin to generate the import headers:

  1. As is, directly import the types from the typing module.
  2. Import the root typing module, and reference the types from the modules
  3. allow for the usage of the python 3.10 style of type definition.

I have also added code to validate the generated modules for conflicts like the one above.

Important

This PR also fundamentaly changes how typing imports are loaded. Instead of trying to be prescriptive and see what imports we will need, the typing compiler tracks all calls that use it, including in the generation of the body of the Jinja Template, then seperatly generates the header and gathers all imports from what is actually used.

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, ...)

Tests:

Poe => pytest
============================= test session starts ==============================
platform darwin -- Python 3.11.9, pytest-6.2.5, py-1.11.0, pluggy-1.2.0
rootdir: /Users/ianmcdonald/git/python-betterproto, configfile: pytest.ini
plugins: cov-2.12.1, pylint-0.21.0, black-0.3.12, common-tools-3.1.48, isort-3.1.0, asyncio-0.12.0, mock-3.11.1
collected 718 items

tests/test_casing.py ................................................... [  7%]
.....................                                                    [ 10%]
tests/test_deprecated.py ....                                            [ 10%]
tests/test_enum.py .................                                     [ 12%]
tests/test_features.py .....................                             [ 15%]
tests/test_get_ref_type.py ............................................. [ 22%]
.......                                                                  [ 23%]
tests/test_inputs.py .......x.....................X.......X....Xx....... [ 30%]
........x.....................X.......X....Xx........................... [ 40%]
.x....X...x..................................x....x...x...               [ 48%]
tests/test_mapmessage.py .                                               [ 48%]
tests/test_module_validation.py ................                         [ 50%]
tests/test_pickling.py .....                                             [ 51%]
tests/test_streams.py .......................sssss                       [ 55%]
tests/test_struct.py .                                                   [ 55%]
tests/test_timestamp.py ....                                             [ 55%]
tests/test_typing_compiler.py ...                                        [ 56%]
tests/test_version.py .                                                  [ 56%]
tests/grpc/test_grpclib_client.py ..............                         [ 58%]
tests/grpc/test_stream_stream.py .....                                   [ 59%]
tests/inputs/bool/test_bool.py ...                                       [ 59%]
tests/inputs/casing/test_casing.py ...                                   [ 60%]
tests/inputs/casing_inner_class/test_casing_inner_class.py ..            [ 60%]
tests/inputs/enum/test_enum.py ...........                               [ 61%]
tests/inputs/example_service/test_example_service.py .                   [ 61%]
tests/inputs/google_impl_behavior_equivalence/test_google_impl_behavior_equivalence.py . [ 62%]
...                                                                      [ 62%]
tests/inputs/googletypes_request/test_googletypes_request.py ........... [ 64%]
                                                                         [ 64%]
tests/inputs/googletypes_response/test_googletypes_response.py ......... [ 65%]
xxxxxxxxx                                                                [ 66%]
tests/inputs/googletypes_response_embedded/test_googletypes_response_embedded.py . [ 66%]
                                                                         [ 66%]
tests/inputs/import_service_input_message/test_import_service_input_message.py . [ 66%]
..                                                                       [ 67%]
tests/inputs/nestedtwice/test_nestedtwice.py ......                      [ 67%]
tests/inputs/oneof/test_oneof.py ....x                                   [ 68%]
tests/inputs/oneof_default_value_serialization/test_oneof_default_value_serialization.py . [ 68%]
..                                                                       [ 69%]
tests/inputs/oneof_enum/test_oneof_enum.py ...                           [ 69%]
tests/inputs/proto3_field_presence/test_proto3_field_presence.py ..      [ 69%]
tests/inputs/proto3_field_presence_oneof/test_proto3_field_presence_oneof.py . [ 69%]
                                                                         [ 69%]
tests/inputs/regression_387/test_regression_387.py .                     [ 70%]
tests/inputs/regression_414/test_regression_414.py .                     [ 70%]
tests/inputs/repeated_duration_timestamp/test_repeated_duration_timestamp.py . [ 70%]
                                                                         [ 70%]
tests/inputs/service_uppercase/test_service.py .                         [ 70%]
tests/inputs/timestamp_dict_encode/test_timestamp_dict_encode.py ....... [ 71%]
........................................................................ [ 81%]
........................................................................ [ 91%]
.............................................................            [100%]

============ 687 passed, 5 skipped, 19 xfailed, 7 xpassed in 5.21s =============

@imcdo
Copy link
Contributor Author

imcdo commented Jun 12, 2024

@Gobot1234 would appreciate a review if you get some time and are good with the change.

@imcdo imcdo changed the title Imcdo/typing collisions Handle Typing Collisions and add validation to a files module for overlaping declarations Jun 12, 2024
@imcdo imcdo changed the title Handle Typing Collisions and add validation to a files module for overlaping declarations Handle typing collisions and add validation to a files module for overlaping declarations Jun 12, 2024
@Gobot1234 Gobot1234 merged commit 8b59234 into danielgtaylor:master Jul 19, 2024
19 checks passed
@Gobot1234
Copy link
Collaborator

Thanks!

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.

3 participants