fix: Remove unnecessary connection test and add close/close_async methods to WeaviateDocumentStore#2891
Conversation
WeaviateDocumentStoreclose/close_async methods to WeaviateDocumentStore
anakin87
left a comment
There was a problem hiding this comment.
Thank you for the contribution!
I left some comments.
| ) | ||
| yield store | ||
| store.client.collections.delete(collection_settings["class"]) | ||
| store.close() |
There was a problem hiding this comment.
A lot of the async tests call write_documents which uses the sync client so I think it is yes.
There was a problem hiding this comment.
Ah, I see. This document store does not have the write_documents_async method...
I'll open an issue to fill this gap.
|
Thanks for the feedback and review @anakin87, it's much appreciated I'll make sure to be more thorough in the tests I add in future PRs. I believe I have addressed all of your comments now :) |
| @@ -1,4 +1,3 @@ | |||
| version: '3.4' | |||
There was a problem hiding this comment.
I would not remove this line, unless it's needed for some reason
There was a problem hiding this comment.
Ah I see it's now an obsolete field that can be safely removed
anakin87
left a comment
There was a problem hiding this comment.
Thank you. Looks good!
I'll make sure to be more thorough in the tests I add in future PRs.
I appreciate your contributions and interactions: it's no small thing in these times dominated by AI agents 🙂
Related Issues
Proposed Changes:
self._client.connect()/self._async_client.connect()is called allowing bothself._client.collections.list_all(simple=True)andself._async_client.collections.list_all(simple=True)to be removed.closeandasync_closemethods toWeaviateDocumentStoremirroring what is done inValkeyDocumentStoreandPineconeDocumentStore.How did you test it?
Modified existing integration tests.
Notes for the reviewer
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.