Add Typeshare support for Python #169

hculea · 2024-04-30T13:24:22Z

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.

@adriangb

Thanks to the prior work of @adriangb: 1Password#25

Lucretiel · 2024-06-04T16:32:39Z

core/Cargo.toml

@@ -14,6 +14,9 @@ thiserror = "1"
 itertools = "0.12"
 lazy_format = "2"
 joinery = "2"
+topological-sort = { version = "0.2.2"}
+once_cell = { version = "1"}


Use the built-in oncelock https://doc.rust-lang.org/std/sync/struct.OnceLock.html

Any reason to not use the built-in type? https://doc.rust-lang.org/stable/std/sync/struct.LazyLock.html

core/src/language/python.rs

Lucretiel · 2024-06-04T19:43:46Z

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

…hare

… and ConfigDict

MOmarMiraj · 2024-10-31T20:50:28Z

Forgot to mention but we currently do not support generics as we have yet to figure out on an ideal/idiomatic way to handle them in Python so we left it out of this PR.

hculea · 2024-11-04T10:32:08Z

Thanks for the mention @MOmarMiraj 😄 That should be okay while Python support is still experimental (FWIW, Go doesn't support generics either)

hculea · 2024-11-04T10:36:53Z

@Lucretiel does @MOmarMiraj 's comment above answer to your high-level description request? Otherwise, we're happy to provide more context on the choices that were made here. Thanks!

…culea/typeshare into hculea/add-python-support-in-typeshare

Fix tuple variant generation

Lucretiel

Mostly this looks good. I had some concerns about algebraic enum representations, along with a few edge cases (mostly around serde renames) that appear to be handled incorrectly. Rust side of this looks fine, if quite allocation-heavy; I'll leave it be for now but there's a lot of low-hanging fruit for improvement if typeshare-python needs to be faster.

Lucretiel · 2024-11-11T05:37:38Z

core/Cargo.toml

@@ -14,6 +14,9 @@ thiserror = "1"
 itertools = "0.12"
 lazy_format = "2"
 joinery = "2"
+topological-sort = { version = "0.2.2"}
+once_cell = { version = "1"}


Any reason to not use the built-in type? https://doc.rust-lang.org/stable/std/sync/struct.LazyLock.html

core/data/tests/anonymous_struct_with_rename/output.py

core/data/tests/can_apply_prefix_correctly/output.py

core/data/tests/test_algebraic_enum_case_name_support/output.py

Lucretiel · 2024-11-11T06:01:43Z

core/data/tests/test_generate_char/output.py

+
+
+class MyType(BaseModel):
+    field: str


Consider constr to constrain the length here, since it's known to be a char https://docs.pydantic.dev/1.10/usage/types/#arguments-to-constr

Will be using Annotated with Field instead.

Lucretiel · 2024-11-11T06:02:24Z

core/data/tests/test_optional_type_alias/output.py

+OptionalU16 = int
+
+
+OptionalU32 = int
+
+
+class FooBar(BaseModel):
+    foo: OptionalU32
+    bar: OptionalU16


This looks very wrong to me– the relevant rust type is Option<u32>

Lucretiel · 2024-11-11T06:05:23Z

core/data/tests/use_correct_integer_types/output.py

+    """
+    This is a comment.
+    """
+    a: int


Nit: I don't feel nearly as strongly about this as I do about by other feedback, but if you wanted to be fancy, you could use conint to handle fixed-width integers with a min and max value: https://docs.pydantic.dev/1.10/usage/types/#arguments-to-conint

Will be using Annotated with Field instead, like so: Annotated[int, Field(strict=True, gt=0)]

Lucretiel · 2024-11-11T06:08:05Z

core/src/language/python.rs

+// used by each individual field
+// We do this by exploring each field's type and comparing against the generics used by the enum
+// itself
+fn collect_generics_for_variant(variant_type: &RustType, generics: &[String]) -> Vec<String> {


Nit: consider using a wrapper that pushes to an &mut Vec<String> here, to avoid the deep recursive frequent allocations and data copying between vectors. Also consider returning a HashSet or BTreeSet from this function.

This function is unused from the original contributor. This PR won't cover generics so I'm keeping the function around for future references.

I will add a comment a mark the as unused.

Lucretiel · 2024-11-11T06:19:53Z

Follow-up: it looks like most of the pydantic con* types (conlist, constr, etc) are deprecated, in favor of Field and Annotated, so substitute all my recommendations as appropriate.

Lucretiel · 2024-11-11T06:24:35Z

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.

…lly tagged

CheatCod · 2024-11-14T01:50:38Z

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 · 2024-11-15T00:36:14Z

core/data/tests/anonymous_struct_with_rename/output.py

+    KEBAB_CASE = "kebabCase"
+
+class AnonymousStructWithRenameList(BaseModel):
+    type: AnonymousStructWithRenameTypes = AnonymousStructWithRenameTypes.LIST


It's possible I'm misunderstanding pydantic, but shouldn't this be a Literal parameter? I had understood this = to just set a default value.

https://docs.pydantic.dev/2.0/usage/types/unions/

Add experimental typeshare support for Python

90f7537

Thanks to the prior work of @adriangb: 1Password#25

hculea marked this pull request as draft April 30, 2024 13:24

kareid requested a review from Lucretiel May 1, 2024 15:04

hculea added 3 commits May 28, 2024 16:45

Resolve clippy errors

ee5673c

Resolve merge conflicts with latest main

78bca98

Resolve merge conflicts with latest main

dfd6522

hculea marked this pull request as ready for review May 28, 2024 15:01

Add python as an option for the --lang flag

0c14914

Lucretiel reviewed Jun 4, 2024

View reviewed changes

core/src/language/python.rs Outdated Show resolved Hide resolved

Lucretiel reviewed Jun 4, 2024

View reviewed changes

core/src/language/python.rs Outdated Show resolved Hide resolved

Lucretiel reviewed Jun 4, 2024

View reviewed changes

core/src/language/python.rs Outdated Show resolved Hide resolved

Lucretiel reviewed Jun 4, 2024

View reviewed changes

core/src/language/python.rs Outdated Show resolved Hide resolved

hculea added 3 commits June 10, 2024 14:58

Add support for todo and priorly unsupported types

439783b

Leverage mutable self to remove RefCell around Module

d7f12d0

Fix ParsedRusthThing typo

2039818

MOmarMiraj force-pushed the hculea/add-python-support-in-typeshare branch from f4540ab to 85634d4 Compare July 31, 2024 19:42

Merge branch 'main' into hculea/add-python-support-in-typeshare

48a8adb

MOmarMiraj force-pushed the hculea/add-python-support-in-typeshare branch from 85634d4 to 48a8adb Compare July 31, 2024 19:42

AndyTitu and others added 5 commits August 5, 2024 11:11

Merge branch '1Password:main' into hculea/add-python-support-in-types…

1427223

…hare

Use serialization alias in renaming fields in Python

c9f4be4

Merge branch '1Password:main' into hculea/add-python-support-in-types…

ad6645a

…hare

Fix python serializing and deserialzing using pydantic Field aliasing…

459f596

… and ConfigDict

Fix indentation of writing config dict

fde8907

amandayu1 force-pushed the hculea/add-python-support-in-typeshare branch from fbfde61 to 5c88f19 Compare August 22, 2024 18:16

added None default to Optional python type

ead7d3a

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

Add mention of Python support in README

3d447df

hculea requested a review from Lucretiel November 4, 2024 10:37

MOmarMiraj and others added 9 commits November 6, 2024 21:26

remove trailing comma

a251db1

Merge branch 'hculea/add-python-support-in-typeshare' of github.com:h…

03d600b

…culea/typeshare into hculea/add-python-support-in-typeshare

Update unit enum to use enum values

61b6afd

fmt fix

9810e7b

Remove config dict and allow unit enums to be serializble

92f6922

fix tuple variant generation

0f89783

dedup union

90793c7

better error handling

40803c4

Merge pull request #4 from hculea/peter/fix-tuple-variant-generation

23ecf7f

Fix tuple variant generation

Lucretiel requested changes Nov 11, 2024

View reviewed changes

CheatCod added 4 commits November 12, 2024 10:13

remove once-cell; use index-set

ce97007

update tests

260fa75

add comments on unused function for generics

d4fabb6

refactor how union works. Note this requires the struct to be externa…

83001f7

…lly tagged

proper rename

6afd25a

Lucretiel reviewed Nov 15, 2024

View reviewed changes

CheatCod added 6 commits November 15, 2024 11:45

use literal for tag keys

0c9b8ac

generate comments properly

a10d3e7

fix type aliasing issue

3df3ece

remove unneeded newlines

533c75f

fix to confrom python's wonderful doc conventions

4a0bb0e

use enum for type

a49914a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Typeshare support for Python #169

Add Typeshare support for Python #169

hculea commented Apr 30, 2024 •

edited

Loading

Lucretiel Jun 4, 2024

Lucretiel Nov 11, 2024

Lucretiel commented Jun 4, 2024

MOmarMiraj commented Oct 31, 2024

hculea commented Nov 4, 2024

hculea commented Nov 4, 2024

Lucretiel left a comment

Lucretiel Nov 11, 2024

Lucretiel Nov 11, 2024

CheatCod Nov 12, 2024

Lucretiel Nov 11, 2024

CheatCod Nov 15, 2024

Lucretiel Nov 11, 2024

CheatCod Nov 12, 2024

Lucretiel Nov 11, 2024

CheatCod Nov 12, 2024

Lucretiel commented Nov 11, 2024

Lucretiel commented Nov 11, 2024

CheatCod commented Nov 14, 2024

Lucretiel Nov 15, 2024

CheatCod Nov 15, 2024

Add Typeshare support for Python #169

Are you sure you want to change the base?

Add Typeshare support for Python #169

Conversation

hculea commented Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lucretiel commented Jun 4, 2024

MOmarMiraj commented Oct 31, 2024

hculea commented Nov 4, 2024

hculea commented Nov 4, 2024

Lucretiel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lucretiel commented Nov 11, 2024

Lucretiel commented Nov 11, 2024

CheatCod commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hculea commented Apr 30, 2024 •

edited

Loading