Conversation
|
any plans to merge this any time soon? |
|
|
||
|
|
||
| def _encode_pydantic(obj): | ||
| from pydantic.json import pydantic_encoder |
There was a problem hiding this comment.
I think that here is better to auto-detect the version of pydantic that is installed and import the correct function so we keep backward compatibility.
There was a problem hiding this comment.
Hm, good point. Pydantic claims they will stop supporting V1 when V3 is released (somewhere this year), so I am not sure if V1 support gonna be needed.
What I missed is explicitly requiring a pydantic>2 for uplink - that's fixed now
|
@djbios can you add a test for a model inheriting from |
|
IMHO we better keep this implementation as is for v1 and/or for the |
I'm not sure that can happen IRL (user has v2 installed but uses V1 models), but to support that it would require extra changes
I agree backward compatibility would be great, just not sure how to unit test it properly in the current setup. |
No, v2 has a copy of v1 that can be accessed as |
|
That's why I was doing https://github.com/prkumar/uplink/pull/295/files, maybe we don't have to support the old pydantic, but keeping support for the embedded |
That's true, but as I mentioned, v1's BaseModel won't work with v2's |
|
I will make a PR with my proposal, I think my idea is clearer in code than in a comment 😄 |
|
I don't think this project is still maintained. Would you guys consider forking it and publishing it? |
| # For Python 3.5 and newer | ||
| async def coroutine(): | ||
| return mock_response | ||
| elif (3, 3) <= sys.version_info < (3, 5): |
There was a problem hiding this comment.
Might be safe to drop this. I don't believe we officially support Python versions below 3.5
| import asyncio | ||
| # Check the Python version and define coroutine accordingly | ||
| if sys.version_info >= (3, 5): | ||
| # For Python 3.5 and newer |
There was a problem hiding this comment.
non-blocking: I would expect this to throw a SyntaxError for Python 2.7. But, we are way beyond EOL now, and should drop 2.7 support IMO.
@leiserfg - added this same question to your PR: f I understand correctly, you are saying that Pydantic v2 includes a v1 package so that users could upgrade to v2 without needing to migrate their usage to the v2 BaseModel directly. Is that right? And, in that scenario, a package with pydantic 2 installed may still be using v1 BaseModel. |
There was a problem hiding this comment.
@djbios - Thanks for putting this together! I also see @leiserfg sent #312.
There is possible path forward where we stack #312 on top of this, keep this one focused on adding v2 support, and focus #312 on extending it to support v1.
@djbios and @leiserfg - let me know if that path sounds good to both of you. Otherwise, let's pick one PR to focus on and commit.
Thank you both!
|
What do you think of using my pr for the pydantic V2 stuff and keeping from this one the async part only? |
|
@leiserfg I guess if we decide to drop old Python versions support - the async part is not needed anymore. We should update the docs/setup.py if so though |
6 participants
Fixes #297
Changes proposed in this pull request:
Attention: @prkumar
If this will be merged, #298 is not needed.