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

order_by and limit params to find() #51

Merged
merged 5 commits into from
Feb 26, 2024

Conversation

antont
Copy link
Contributor

@antont antont commented Nov 9, 2023

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.

…g this to test the version with limit & order-by params in later PRs.
…nd to IMHO make it a little 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.
@antont antont mentioned this pull request Nov 13, 2023
@fbjorn
Copy link
Contributor

fbjorn commented Feb 13, 2024

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
Copy link
Contributor Author

antont commented Feb 13, 2024

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
Copy link
Contributor

This is continued in this PR

joakimnordling added a commit that referenced this pull request Feb 26, 2024
Add order_by, limit and offset as options in find query parameters
Updated urllib3 for fix vulnerabilities

Continuation of work from #51 and fixes #52 and #53
@joakimnordling joakimnordling merged commit 9f476f8 into ioxiocom:main Feb 26, 2024
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.

None yet

5 participants