feat: Document Loader for Datastore. #7
Conversation
| TYPE = "datastore_type" | ||
|
|
||
|
|
||
| class DocumentConverter: |
There was a problem hiding this comment.
Why do we need a DocumentConverter class here? What value does it add over these staticmethods just being functions?
| def convertFirestoreEntity( | ||
| entity: Entity, | ||
| page_content_properties: List[str] = [], | ||
| metadata_properties: List[str] = [], | ||
| ) -> Document: |
There was a problem hiding this comment.
Can we add some comments? convertFirestoreEntity -- am I converting to a FirestoreEntity? from a FirestoreEntity?
There was a problem hiding this comment.
Same for all 4 of these functions
| val_converted = { | ||
| k: DocumentConverter._convertFromLangChain(v, client) | ||
| for k, v in val.items() | ||
| } | ||
| elif isinstance(val, list): | ||
| val_converted = [ | ||
| DocumentConverter._convertFromLangChain(v, client) for v in val | ||
| ] |
There was a problem hiding this comment.
Are these for covering the nested cases?
It would probably be more readable to short circuit these at the top
There was a problem hiding this comment.
SG. I will move this to the TOP in both PRs
| @pytest.fixture | ||
| def test_case() -> TestCase: | ||
| return TestCase() |
There was a problem hiding this comment.
What is this being used for?
There was a problem hiding this comment.
For the assert, that ensures two list have the same elements. The name in python3 is misleading:
test_case.assertCountEqual
https://docs.python.org/3.6/library/unittest.html#unittest.TestCase.assertCountEqual
Previously called assertItemsEqual
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Add Document Loader for Firestore in Datastore Mode