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

Fixes, AsyncAbstractRepository and more #45

Closed
wants to merge 37 commits into from

Conversation

Artucuno
Copy link
Contributor

@Artucuno Artucuno commented Nov 23, 2023

Fix small issue with not having any models to insert when using `save_many()`
Add `delete_many()`, update `save_many()`, update type hints, and add document_count property.
@Artucuno
Copy link
Contributor Author

Just noticed that my IDE indented a bunch of stuff for no reason. It doesn't seem to interfere with anything but I can fix it if needed.

@Artucuno
Copy link
Contributor Author

Artucuno commented Nov 26, 2023

Also fixed the weird indents from 7a82aae

@Artucuno Artucuno changed the title Add delete_many() and document_count property Fixes, delete_many() and document_count Nov 27, 2023
@Artucuno
Copy link
Contributor Author

7ea5048 validated with:

Click me
from bson import ObjectId
from fastapi import FastAPI
from icecream import ic
from pydantic import BaseModel

from pydantic_mongo import ObjectIdField


class Foo(BaseModel):
    id: ObjectIdField = None
    message: str


app = FastAPI()


@app.get("/", response_model=Foo)
async def root():
    return {"id": ObjectId(), "message": "Hello World"}


bar = Foo(id=ObjectId(), message="Hello World")

md = bar.model_dump()
ic(md)
ic(type(Foo.model_validate(md).id))

mdj = bar.model_dump_json()
ic(mdj)
ic(type(Foo.model_validate_json(mdj).id))

if __name__ == '__main__':
    import uvicorn

    uvicorn.run(app, host="0.0.0.0", port=8000)

Results with:

ic| md: {'id': '6567d22651631f1e5982e518', 'message': 'Hello World'}
ic| type(Foo.model_validate(md).id): <class 'bson.objectid.ObjectId'>
ic| mdj: '{"id":"6567d22651631f1e5982e518","message":"Hello World"}'
ic| type(Foo.model_validate_json(mdj).id): <class 'bson.objectid.ObjectId'>

while maintaining OpenAPI compatibility
image

@Artucuno
Copy link
Contributor Author

Artucuno commented Dec 1, 2023

Hey @jefersondaniel, Is there anything that I need to do before you merge?

Thanks!

@Artucuno Artucuno mentioned this pull request Dec 3, 2023
@Artucuno Artucuno changed the title Fixes, delete_many() and document_count Fixes, AsyncAbstractRepository and more Dec 6, 2023
@jefersondaniel
Copy link
Owner

jefersondaniel commented Jan 5, 2024

Thanks @Artucuno,

I'm not sure about the best implementation for kwargs, I want to keep type hints whenever possible. What do you think about the typehints for kwargs?

You can send kwargs as a separate PR so I can merge the other changes first

@Artucuno
Copy link
Contributor Author

I'm not sure about the best implementation for kwargs, I want to keep type hints whenever possible. What do you think about the typehints for kwargs?

Sorry for the late response. What do you mean about typehints for kwargs? Since the kwargs are being passed directly to pymongo, I don't think that it would need typehinting.

@Artucuno Artucuno marked this pull request as draft January 28, 2024 07:10
@Artucuno
Copy link
Contributor Author

Temporarily converted this to a draft to fix some things

@Artucuno
Copy link
Contributor Author

Artucuno commented Jan 28, 2024

I don't think that the README or Linting tests are necessary. The linting should be a Github action instead, but its also broken.

# The broken test in phulpyfile.py
@task
def lint(phulpy):

image

image

@Artucuno
Copy link
Contributor Author

Omg finally

@Artucuno Artucuno marked this pull request as ready for review January 28, 2024 09:35
@Artucuno
Copy link
Contributor Author

@jefersondaniel Is there anything else that you need me to do?

Thanks,
Artu

@jefersondaniel
Copy link
Owner

Hi @Artucuno,

Thank you for the effort and time you've put into this PR. appreciate some of the ideas you've introduced, such as delete_many and AsyncAbstractRepository. These are great additions.

However, I have some concerns about the scope of this PR. I generally prefer to work with single-purpose PRs as they're easier to review, test, and discuss.

I also have some reservations about the extensive use of kwargs in some of the changes. While kwargs can offer flexibility, their overuse might compromise the code's maintainability. This approach increases coupling with implementation details, as callers need to have a understanding of how the function is implemented and where the kwargs are being utilized.

I'm closing this PR for now, but I want to incorporate some of your changes. If you have any further thoughts or decide to submit smaller PRs in the future, I would be more than happy to review them. Otherwise I'll review this code more carefully and try to cherry-pick some of the changes.

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.

2 participants