search

sopherapps / pydantic-redis

A simple Declarative ORM for Redis using pydantic Models
https://sopherapps.github.io/pydantic-redis
MIT License
43 stars 14 forks source link

Handle models in List #7

Closed Rictus closed 2 years ago

Rictus commented 2 years ago

Hello,

I have a case where a pydantic_redis model could not be serialized by orjson whereas pydantic could. This allows to fallback and if pydantic can't serialize, it will throw the original TypeError so that's fine. Very plug&play.

Tell me if you want me to move that function somewhere else. [Edit] Issue #8

Tinitto commented 2 years ago

@Rictus Thanks for the pull request.

Could you create an issue describing that bug. This helps us in case such a model breaks more stuff than just serializing. Please also share a sample model so that we can also update our test suit.

Rictus commented 2 years ago

@Tinitto I've got a working Serialization/Deserialization method for when there is a list of models as a property of a parent Model.

Let me know what you think. We could expand the idea to sets, dicts and tuples but for now, list is all I need.

Rictus commented 2 years ago

Hello @Tinitto I'm kindly requesting you to review my work to validate this is the right direction

Thanks

Tinitto commented 2 years ago

@Rictus I am so sorry. I have just noticed your comment. I had somehow missed it. Let me have a look-see.

Rictus commented 2 years ago

Hello @Tinitto Thank you for your suggestions, the integration is now much cleaner.

Tinitto commented 2 years ago

Oh... okay. I hope to have a look a later today.

Rictus commented 2 years ago

I think there is more work to do to not end up processing list of strings as a list of nested models

Tinitto commented 2 years ago

Hey @Rictus, could you remove the code for anything other than List[Model]...the more specific this pull request is, the easier it will be to implement and review. You could put it in a different branch or something.

Rictus commented 2 years ago

Hey @Rictus, could you remove the code for anything other than List[Model]...the more specific this pull request is, the easier it will be to implement and review. You could put it in a different branch or something.

Sure no problem. I have implemented it only because you requested it earlier. Maybe I didn't understood you correctly

I only need list for my use cases.

Tinitto commented 2 years ago

Hey @Rictus, could you remove the code for anything other than List[Model]...the more specific this pull request is, the easier it will be to implement and review. You could put it in a different branch or something.

Sure no problem. I have implemented it only because you requested it earlier. Maybe I didn't understood you correctly

I only need list for my use cases.

:-) yeah...I started rumbling and I confused you.

Rictus commented 2 years ago

Hey @Tinitto How far are we to merge this PR ? I have integrated your ideas from your PR on my repo.

I have not integrated the changes on the ci.yml file as it is not really related to this PR.

Thanks

Tinitto commented 2 years ago

Let me check one last time.

Tinitto commented 2 years ago

@Rictus , the code looks good. The formatting was not uniform so I added black....https://github.com/psf/black and now the CI is failing. So all that is left is to add black, run its formatter, commit and push.

pip install black
black .
Rictus commented 2 years ago

@Tinitto Done 👍