-
Notifications
You must be signed in to change notification settings - Fork 64
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
Update pydantic samples to use contrib module #163
Conversation
pydantic_converter/v1/README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer a pydantic_converter_v1
sample instead of putting inside another sample
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v1's a thing of the past. I think that pollutes the top-level namespace unnecessarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's easier to ignore a top-level sample you don't care about than code inside a sample you do. This looks like v1
is a subdir/module of the pydantic_converter
sample, not its own sample. Subdirs of Python code are usually considered modules of that code, not separate/unrelated (and this has been the case in samples thus far).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK you're probably right about the structure. But let's delete it at some point -- when we do we can always leave a permalink to the content in git history in the remaining README of the v2 sample.
This reverts commit 67e20be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would move this to tests/pydantic_converter_v1
dir
To merge once temporalio/sdk-python#757 is released