Skip to content

Add delete documents by #44

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

Merged
merged 6 commits into from
Nov 4, 2020

Conversation

widlok
Copy link

@widlok widlok commented Oct 22, 2020

#42 added implementation and tests.
Regards Chris

kination
kination previously approved these changes Oct 26, 2020
@kination
Copy link
Contributor

Seems good~ @eskombro how do you think?

@eskombro
Copy link
Member

eskombro commented Oct 26, 2020

Great, Thanks for the PR @widlok and @djKooks for the review!

As @widlok said in #42 there is a problem with the integration test:

com.meilisearch.integration.DocumentsTest > testDeleteDocuments() FAILED
    java.lang.IllegalStateException at DocumentsTest.java:218

Can you please check that?

Let me know when it is working or if you need any help :)

And remember you can run the integration tests locally with the tag "integration"

@widlok
Copy link
Author

widlok commented Oct 27, 2020

Thanks for reply @eskombro . I run those tests before posting but for some reason all tests fail with error

Connection refused: connect java.net.ConnectException: Connection refused: connect

at com.meilisearch.sdk.http.DefaultHttpClient.execute(DefaultHttpClient.java:53)
	at com.meilisearch.sdk.http.DefaultHttpClient.post(DefaultHttpClient.java:73)
	at com.meilisearch.sdk.MeiliSearchHttpRequest.post(MeiliSearchHttpRequest.java:38)
	at com.meilisearch.sdk.IndexesHandler.create(IndexesHandler.java:46)
	at com.meilisearch.sdk.IndexesHandler.create(IndexesHandler.java:28)
	at com.meilisearch.sdk.Client.createIndex(Client.java:36)
	at com.meilisearch.integration.ClientTest.testDeleteIndex(ClientTest.java:109)

This is log from just one test testDeleteIndex() but all tests fail, on method call createIndex(...), with this error.

@eskombro
Copy link
Member

eskombro commented Oct 27, 2020

Connection refused: connect java.net.ConnectException: Connection refused: connect

Is your MeiliSearch instance running? Seems to be down if every test fails because of a connection exception

@widlok
Copy link
Author

widlok commented Oct 28, 2020

I develop on windows, i cant find server option of Meilisearch for windows 10. How can i test this project on win 10?

@eskombro
Copy link
Member

@widlok you can download the windows binary from the MeiliSearch Release page!

You can also use docker, or create a remote instance in MeiliSearch Sandbox!

@widlok
Copy link
Author

widlok commented Oct 29, 2020

Thanks for advice! Docker looks most convenient , new PR up for a review :)

eskombro
eskombro previously approved these changes Nov 3, 2020
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 the PR @widlok !

I see that you refactored a little bit the test classes, its ok 👍

Is there any reason to prefer using a List of identifiers instead of an Array? This is out of curiosity, because as I have stated, I'm no java expert, and I'm interested in understanding those choices, since soon it will be important to check that there is a coherence between the methods and the data types we use on them, and choices will have to be consistent

Thank you very much! Tests work perfectly now 👌

@eskombro
Copy link
Member

eskombro commented Nov 3, 2020

Can you please rebase your branch on master and ping me when it's all good?

@widlok
Copy link
Author

widlok commented Nov 4, 2020

Is there any reason to prefer using a List of identifiers instead of an Array? This is out of curiosity, because as I have stated, I'm no java expert, and I'm interested in understanding those choices, since soon it will be important to check that there is a coherence between the methods and the data types we use on them, and choices will have to be consistent

Operating on Arrays can be a bit cumbersome in Java. Lists and Streams have fancy one liners to iterate through it, filter it, map it etc. which makes code immensely more readable and reduce number of code lines . You could use Arrays over List if iterating over it becomes performance bottleneck but nowadays most bottlenecks are I/O operations more than CPU operations.
Cheers Chris.

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, Thanks a lot!

@eskombro eskombro merged commit b3d55b7 into meilisearch:master Nov 4, 2020
@eskombro
Copy link
Member

eskombro commented Nov 4, 2020

Thanks for your explanation @widlok! :)

A note: I just realized after merging that your commits are being registered as anonymous commits (and not listed in the contributors), maybe a missconfiguration on your git config!

@widlok
Copy link
Author

widlok commented Nov 26, 2020

Thanks for the info @eskombro ! My git tried to force me to become unnamed hero, Its fixed now :)

@eskombro
Copy link
Member

@widlok Oh that nasty Git! :)

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.

3 participants