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

Add Typeshare support for Python #169

Merged
merged 68 commits into from
Dec 9, 2024

Conversation

hculea
Copy link
Member

@hculea hculea commented Apr 30, 2024

This PR is adding initial support for Python in Typeshare (gated by a feature flag, like Go).

See @MOmarMiraj 's comment for a high-level description of the design considerations behind this work.

This was made possible thanks to the prior work of @adriangb: #25.

@hculea hculea marked this pull request as draft April 30, 2024 13:24
@kareid kareid requested a review from Lucretiel May 1, 2024 15:04
@hculea hculea marked this pull request as ready for review May 28, 2024 15:01
core/Cargo.toml Outdated Show resolved Hide resolved
@Lucretiel
Copy link
Contributor

Doing some review, sorry for the delay. I'd like to ask for a high-level description in this MR for how this integration works, with especial focus on how structs are handled (in particular how the JSON <-> struct boundary is handled).

@MOmarMiraj MOmarMiraj force-pushed the hculea/add-python-support-in-typeshare branch from f4540ab to 85634d4 Compare July 31, 2024 19:42
@MOmarMiraj MOmarMiraj force-pushed the hculea/add-python-support-in-typeshare branch from 85634d4 to 48a8adb Compare July 31, 2024 19:42
@amandayu1 amandayu1 force-pushed the hculea/add-python-support-in-typeshare branch from fbfde61 to 5c88f19 Compare August 22, 2024 18:16
@amandayu1 amandayu1 force-pushed the hculea/add-python-support-in-typeshare branch 3 times, most recently from c901b1b to 9935aaf Compare August 22, 2024 20:19
@Lucretiel
Copy link
Contributor

As a subsequent follow up, one idea for a way to handle unions would resemble this:

class MyEnumVariant1:
    type: Literal["Variant1"]
    content: MyEnumVariant1Content

...

MyEnum = Union[MyEnumVariant1, ...]

I'm still a novice around pydantic, so there could certainly be a better way that I'm not aware of. For all I know this pattern is fully built-in to pydantic somewhere and I just don't know where.

@CheatCod
Copy link
Contributor

As a subsequent follow up, one idea for a way to handle unions would resemble this:

class MyEnumVariant1:
    type: Literal["Variant1"]
    content: MyEnumVariant1Content

...

MyEnum = Union[MyEnumVariant1, ...]

I'm still a novice around pydantic, so there could certainly be a better way that I'm not aware of. For all I know this pattern is fully built-in to pydantic somewhere and I just don't know where.

After some experimentation and discussion with @MOmarMiraj, I think this is the best approach for externally tagged enums. I've refactored the PR to do so.

@Lucretiel
Copy link
Contributor

Hey all! I'm ready to approve once the remaining feedback is address (or marked Will Not Fix)

@CheatCod
Copy link
Contributor

CheatCod commented Dec 5, 2024

For future improvement, let me know if I've missed anything.

  1. conint from pydantic can be used for further validation. For example, if the Rust type is u8, then we can leverage conint to bound the number from 0 to 255.

  2. struct and enum with generic parameters are not supported. For example, the test case can_generate_generic_enum is not enabled for Python as it is not supported.

  3. Rust enum must be adjacently tagged, like #[serde(tag = "t", content = "c")], other enum representations, such as internally tagged and untagged are not supported. Read more about enum representation with serde here: https://serde.rs/enum-representations.html. Correct me if I am wrong but I believe this is a limitation in Typeshare itself

  4. Type overriding with the #[typeshare] macro is not supported. See the test can_override_types for details.

@hculea hculea mentioned this pull request Dec 5, 2024
5 tasks
@hculea hculea requested a review from Lucretiel December 5, 2024 11:45
@hculea
Copy link
Member Author

hculea commented Dec 5, 2024

Thanks @CheatCod! I've captured these in #217 to make sure we don't lose track of them once merging this PR. I've resolved all the outstanding conversations after making sure they have either been logged as a future improvement or actually resolved.

@Lucretiel I think this should be ready to go! Let me know if you have any other questions or concerns 👍

Copy link
Member

@darrell-roberts darrell-roberts left a comment

Choose a reason for hiding this comment

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

Left some comments about multiple file generation support.

core/src/language/python.rs Outdated Show resolved Hide resolved
core/src/language/python.rs Show resolved Hide resolved
Copy link
Contributor

@Lucretiel Lucretiel left a comment

Choose a reason for hiding this comment

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

Sorry I didn't approve before I left for the long weekend!

@kareid kareid merged commit b6e6bf1 into 1Password:main Dec 9, 2024
6 checks passed
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.

8 participants