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: support pydantic-version google pb #568

Merged
merged 5 commits into from
Apr 8, 2024

Conversation

ii64
Copy link
Contributor

@ii64 ii64 commented Apr 4, 2024

Summary

When using pydantic_dataclass option, the code-genererated stub still using non-pydantic version of dataclass, since we are not updating the refs, we blow up stack due to recursion, e.g. the use of google.protobuf.Struct.

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
Copy link
Collaborator

This is a breaking change for no real reason. Please can you keep the default lib.google.protobuf as the standard dataclasses version

@ii64
Copy link
Contributor Author

ii64 commented Apr 4, 2024

That's the reason why I keep lib.google.protobuf and lib.google.protobuf.compiler - so that it won't break any existing generated code.

  • lib.google.protobuf will import lib.std.google.protobuf
  • lib.google.protobuf.compiler will import lib.std.google.protobuf.compiler

While pydantic one is just an extra add-on for any pydantic_dataclass generated stub

Also tests are passed

@BRUHItsABunny
Copy link

@Gobot1234 what's broken?

Copy link
Collaborator

@Gobot1234 Gobot1234 left a comment

Choose a reason for hiding this comment

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

Please can you delete the @generated from the top of these files and remind people to include changes from the history (e.g. ii64@5666393 which I can see you've included)

gen.sh Outdated Show resolved Hide resolved
@ii64
Copy link
Contributor Author

ii64 commented Apr 8, 2024

Removed @generated and fix codefmt/linter related error. CI all green @Gobot1234

@Gobot1234
Copy link
Collaborator

Thanks! One last thing, can you add a test for this situation so it doesn't happen again.

@ii64
Copy link
Contributor Author

ii64 commented Apr 8, 2024

Could you try to check and run the CI again? CI is green on my end @Gobot1234

@ii64 ii64 requested a review from Gobot1234 April 8, 2024 22:45
@Gobot1234
Copy link
Collaborator

Thank you!

@Gobot1234 Gobot1234 merged commit c3c2055 into danielgtaylor:master Apr 8, 2024
22 checks passed
@JitPackJoyride JitPackJoyride mentioned this pull request Aug 11, 2024
7 tasks
@ii64 ii64 deleted the for-next-2 branch August 19, 2024 19:22
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