-
Notifications
You must be signed in to change notification settings - Fork 33
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
fix(client): Add one deleteIndex method (#427) #456
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.
The method should also be added in AsyncClient.
It's ok to add the @Nonnull
it was only an internal method that is public now.
@@ -232,6 +232,24 @@ public Task copyIndex( | |||
return copyIndex(srcIndexName, dstIndexName, scopes, RequestOptions.empty); | |||
} | |||
|
|||
public Task deleteIndex(@Nonnull String indexName) throws AlgoliaException { |
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.
Could you add the javadoc for those 2 methods?
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.
Yes right, totally forgot. Will do.
57054fb
to
5f17a00
Compare
@ElPicador I think we're good now. Let me know. |
* | ||
* @param indexName The index name that will be deleted | ||
* @return The associated task | ||
* @throws AlgoliaException |
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.
remove if you do not put a comment
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.
Ok I'll remove it.
* @param indexName The index name that will be deleted | ||
* @param requestOptions Options to pass to this request | ||
* @return The associated task | ||
* @throws AlgoliaException |
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.
same
@@ -648,7 +648,11 @@ public String generateSecuredApiKey( | |||
} | |||
|
|||
/** Package protected method for the Index class */ | |||
CompletableFuture<AsyncTask> deleteIndex(String indexName, RequestOptions requestOptions) { | |||
CompletableFuture<AsyncTask> deleteIndex(@Nonnull String indexName) { |
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.
the method should be public not protected anymore, like above
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.
Ok I wasn't sure 👍
5f17a00
to
40449fd
Compare
Closes #427
Apparently, the method with
RequestOptions
wasn't missing. So here, I'm just adding the one without it, to stay consistent with other methods.Question
However, I have a question: I've also added the
@Nonnull
annotations on the existingdeleteIndex
method. Will it break existing implementations? It will prevent people passingnull
from compiling but this is really a useful check, it's easy to fix and it shouldn't happen in the first place. WDYT?Test plan
As the first method is already tested for both sync and async clients, I have added no tests.