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

Limit redefinition in DocumentsQuery #407

Closed
wants to merge 17 commits into from

Conversation

Azanul
Copy link
Contributor

@Azanul Azanul commented Jan 25, 2023

Pull Request

Related issue

Fixes #331

What does this PR do?

  • Adds support for sending 0 (zero) Limit when querying documents
  • Adapts respective tests
  • Regenerates easyjson file

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@Azanul Azanul marked this pull request as ready for review January 25, 2023 05:49
@Azanul Azanul marked this pull request as draft January 25, 2023 09:23
@brunoocasali
Copy link
Member

@Azanul is this PR finished? Can we review it?

@Azanul
Copy link
Contributor Author

Azanul commented Feb 24, 2023

@brunoocasali I don't know if the discussion in the linked issue was conclusive or not. If so then yes. But let me fix the failing tests first.

@Azanul Azanul marked this pull request as ready for review February 25, 2023 06:28
@alallema
Copy link
Contributor

alallema commented May 9, 2023

Hi @Azanul,

I don't know if the discussion in the linked issue was conclusive or not.

Indeed, the discussion was not concluded, but as said in my comment, I would lean toward the NoLimit global variable option. Again I apologize for the delay 😊

@Azanul
Copy link
Contributor Author

Azanul commented May 13, 2023

@alallema @aldy505 I've updated the implementation and the tests according to global var approach.

@Azanul
Copy link
Contributor Author

Azanul commented May 13, 2023

@brunoocasali Now it's ready for review

index_documents.go Show resolved Hide resolved
Signed-off-by: Azanul <[email protected]>
Copy link

@aldy505 aldy505 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are my suggested changes. You can directly apply it by pressing the "commit suggested changes", otherwise you can retype and commit it yourself.

types.go Outdated Show resolved Hide resolved
index_documents.go Outdated Show resolved Hide resolved
Azanul and others added 2 commits May 14, 2023 12:57
Co-authored-by: Reinaldy Rafli <[email protected]>
Co-authored-by: Reinaldy Rafli <[email protected]>
@Azanul Azanul requested a review from aldy505 May 14, 2023 10:03
@Azanul
Copy link
Contributor Author

Azanul commented Aug 3, 2023

@alallema Seems like newly added Filter code doesn't respect Limit. Do you want me to update respectively.

@brunoocasali
Copy link
Member

@alallema Seems like newly added Filter code doesn't respect Limit. Do you want me to update respectively.

Yes that would be nice @Azanul :)

@Azanul
Copy link
Contributor Author

Azanul commented Aug 9, 2023

@alallema @brunoocasali @aldy505 Do any of you know how I can adapt the current approach for adding 0 Limit support to work with the existing Filter implementation.
I can go back to pointer approach or I'll have to make changes to Filter implementation.

@alallema
Copy link
Contributor

alallema commented Sep 7, 2023

Hi @Azanul,
Thank you so much for all your hard work on this PR.
Unfortunately, we are reducing the support on this repo, and as this issue is not essential and will have to be postponed for Task, Keys etc. I propose to close it.
Thanks again for your involvement and participation ❤️

@alallema alallema closed this Sep 7, 2023
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.

Find a better way to define Limit parameter in all QueryParameter structs
4 participants