igorbenav / fastcrud

FastCRUD is a Python package for FastAPI, offering robust async CRUD operations and flexible endpoint creation utilities.
MIT License
616 stars 43 forks source link

get_multi is not typed #123

Open jakaline-dev opened 2 months ago

jakaline-dev commented 2 months ago

Describe the bug or question This is both an issue and a suggestion that will make breaking changes.

Issue: The returned dict from get_multi is not typed properly. For example,

artist_urls = await artist_url_crud.get_multi(
    db=db_session,
    limit=None,
    return_total_count=False,
    return_as_model=True,
    schema_to_select=ArtistURL,
    artist_id=artist.id,
    source_id=None,
)
for artist_url in artist_urls["data"]: #artist_url is not typed as list[ArtistURL] but 'any'

Request: Also, when return_total_count is False and return_as_model is True, I don't see any reason why the returned object has to be wrapped with a 'data' key. While this works great when returned with FastAPI's API endpoint directly, I also use it in a lot of async functions and this is bugging me.

artist_urls = await artist_url_crud.get_multi(
    db=db_session,
    limit=None,
    return_total_count=False,
    return_as_model=True,
    schema_to_select=ArtistURL,
    artist_id=artist.id,
    source_id=None,
)
for artist_url in artist_urls: # Why can we not do this?
igorbenav commented 1 month ago

Describe the bug or question This is both an issue and a suggestion that will make breaking changes.

Issue: The returned dict from get_multi is not typed properly. For example,

artist_urls = await artist_url_crud.get_multi(
    db=db_session,
    limit=None,
    return_total_count=False,
    return_as_model=True,
    schema_to_select=ArtistURL,
    artist_id=artist.id,
    source_id=None,
)
for artist_url in artist_urls["data"]: #artist_url is not typed as list[ArtistURL] but 'any'

I'll work on it

igorbenav commented 1 month ago

Request: Also, when return_total_count is False and return_as_model is True, I don't see any reason why the returned object has to be wrapped with a 'data' key. While this works great when returned with FastAPI's API endpoint directly, I also use it in a lot of async functions and this is bugging me.

artist_urls = await artist_url_crud.get_multi(
    db=db_session,
    limit=None,
    return_total_count=False,
    return_as_model=True,
    schema_to_select=ArtistURL,
    artist_id=artist.id,
    source_id=None,
)
for artist_url in artist_urls: # Why can we not do this?

What exactly is the issue of doing

for artist_url in artist_urls["data"]

instead?

I guess we could add a parameter for raw returns, but it will add some complexity to the code when a simple artist_urls["data"] could be used

jakaline-dev commented 1 month ago

I guess we could add a parameter for raw returns, but it will add some complexity to the code when a simple artist_urls["data"] could be used Understandable, this is a design choice but I feel this is unintuitive.

For instance, the .get function with return_as_model=True will return the Schema instance itself. This is good. So, I have assumed .get_multi with return_as_model=True would return a list of Schema instances. However, currently it is returning a dict with a single 'data' key (when return_total_count=False)

If return_total_count=True, it makes sense to return a dict we want to get both the 'data' and the 'count'. But if return_total_count=False, I think it should return just a list. Making another parameter for this also feels redundant, so if you have too much to change for this, might as just leave it as it is.