ioxiocom / firedantic

Database models for Firestore using Pydantic base models.
BSD 3-Clause "New" or "Revised" License
43 stars 14 forks source link

order_by and limit params to find() #51

Closed antont closed 5 months ago

antont commented 9 months ago

includes a little ordering in find to IMHO make it a bit clearer, allowing more type declarations, even though it's still not perfect.

docs not added yet, is an API proposal for consideration.

have been using this a couple of weeks in production happily.

fbjorn commented 6 months ago

Hi @antont,

First of all, thanks a lot for your contribution and I'm really sorry it has taken us so long time to reply.

We had a look at it and the PR looks really good. However, by checking the Firestore docs we noticed that a query can contain multiple order_by statements. Thus, we would like to make the order_by a list of tuples, instead of just a tuple. For example:

_OrderDirection = Union[Literal["ASCENDING"], Literal["DESCENDING"]]
_OrderBy = List[tuple[str, _OrderDirection]]

def find(order_by: Optional[_OrderBy]):
    pass

find(order_by=[("created", Query.ASCENDING)])
find(order_by=[("created", Query.ASCENDING), ("name", Query.DESCENDING)])

The example above also tries to increase readability of the type annotations.

Before approving and merging this PR, we would like to see the changes in order_by, updated doc strings and examples added to the README.

It's been a long time since you opened this PR, so we would like to understand whether you're still interested in making these changes, and if you have time for that. Otherwise, we can create a new branch on top of yours and make the changes ourselves, preserving the history, so you will still be a contributor of the project.

How does this sound to you?

antont commented 6 months ago

Hi, great to hear, thanks! And no problem, we've been using my fork happily.

For me it'd be good if you just take the branch/PR and modify like you said. I could also, but do have my plate full now with other things, and you already studied the underlying API (good catch there!) and planned it.

So please just go ahead! I'm happy to adapt our application code to the changes then.

TheHelias commented 6 months ago

This is continued in this PR