-
Notifications
You must be signed in to change notification settings - Fork 26
Fix batch delete for string document ids. (#330) #331
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aronbudinszky, thanks for one more contribution to Meilisearch ❤️
I'm willing to merge your PR, but I would like to avoid breaking for the users currently calling the method with integers.
Do you have any idea whether to accept both types at the same type? Or even create a new method for that?
Oh very good point! Will update that tomorrow by adding a bridging method along with a deprecation notice. |
Hi @brunoocasali! I readded the [Int] methods along with unit tests. Since using Int ids is "wrong" I've also added a deprecation notice so that people who use the old method will get notified to change to string values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing job @aronbudinszky ❤️
bors merge
331: Fix batch delete for string document ids. (#330) r=brunoocasali a=aronbudinszky # Pull Request ## What does this PR do? Fixes #330 ## PR checklist Please check if your PR fulfills the following requirements: - [X] Does this PR fix an existing issue? - [X] Have you read the contributing guidelines? - [X] Have you made sure that the title is accurate and descriptive of the changes? Co-authored-by: Aron Budinszky <[email protected]>
Timed out. |
bors merge |
331: Fix batch delete for string document ids. (#330) r=brunoocasali a=aronbudinszky # Pull Request ## What does this PR do? Fixes #330 ## PR checklist Please check if your PR fulfills the following requirements: - [X] Does this PR fix an existing issue? - [X] Have you read the contributing guidelines? - [X] Have you made sure that the title is accurate and descriptive of the changes? Co-authored-by: Aron Budinszky <[email protected]>
Timed out. |
bors merge |
331: Fix batch delete for string document ids. (#330) r=brunoocasali a=aronbudinszky # Pull Request ## What does this PR do? Fixes #330 ## PR checklist Please check if your PR fulfills the following requirements: - [X] Does this PR fix an existing issue? - [X] Have you read the contributing guidelines? - [X] Have you made sure that the title is accurate and descriptive of the changes? Co-authored-by: Aron Budinszky <[email protected]>
Timed out. |
Hello @aronbudinszky thanks a lot for your contribution 😄 @brunoocasali I've opened a PR to fix this bors issue 😇 |
333: Fix bors.toml r=brunoocasali a=curquiza To avoid time out like in this PR #331 Co-authored-by: Clémentine Urquizar - curqui <[email protected]>
bors merge |
Pull Request
What does this PR do?
Fixes #330
PR checklist
Please check if your PR fulfills the following requirements: