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.

Sign up for GitHub

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

Jump to bottom

Fix 🐛 Documents.List() #31

Merged
merged 2 commits into from
May 18, 2020
Merged

Fix 🐛 Documents.List() #31

merged 2 commits into from
May 18, 2020

Conversation

derekahn
Copy link
Contributor

Problem

It always returned all documents regardless of query params set

Solution

perform request with the required query params vs as a payload

@derekahn
Fix 🐛 Documents.List()
db1587b 
- Problem: it always returned all documents regardless of query params set
- Solution: perform request with the required query params vs as a payload
@curquiza curquiza requested review from eskombro and alexisvisco May 18, 2020 08:41
@curquiza
Copy link
Member

curquiza commented May 18, 2020 •
edited
Loading

Hello @derekahn! Thanks for noticing this issue and fixing it! 😁
I've just requested the author of this package and a Go lover of the Meili team.

eskombro
eskombro requested changes May 18, 2020
View reviewed changes
Copy link
Member

@eskombro eskombro left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, interesting that we haven't seen this behavior before, and thanks for adding a simple test to catch it. I just added a simple suggestion for the test, but your PR seems to work as intended!

client_documents_test.go Outdated Show resolved Hide resolved
@derekahn @eskombro
Update client_documents_test.go
22ce0d4 
Co-authored-by: Samuel Jimenez <sjimenezre@gmail.com>
eskombro
eskombro approved these changes May 18, 2020
View reviewed changes
Copy link
Member

@eskombro eskombro left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@eskombro eskombro merged commit 9375223 into meilisearch:master May 18, 2020
@eskombro
Copy link
Member

eskombro commented May 18, 2020 •
edited
Loading

@derekahn thanks for your contribution! 🎉
@alexisvisco I merged this PR because I wanted to fix the issue fast enough, but if you have any comment, feedback or anything to add, don't hesitate to come back over it! 👍

@derekahn
Copy link
Contributor Author

Thank you guys for all of your hard work on this for people like myself can use ❤️.

alexisvisco
alexisvisco approved these changes May 19, 2020
View reviewed changes
Copy link
Contributor

@alexisvisco alexisvisco left a comment

Choose a reason for hiding this comment

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

Seems okay good jobs and thanks you for the catch :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexisvisco alexisvisco alexisvisco approved these changes

@eskombro eskombro eskombro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@derekahn @curquiza @eskombro @alexisvisco