Branch 3.0.0 v1 #57
Branch 3.0.0 v1 #57
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #57 +/- ##
==========================================
+ Coverage 71.30% 71.46% +0.15%
==========================================
Files 25 25
Lines 2255 2278 +23
==========================================
+ Hits 1608 1628 +20
- Misses 647 650 +3 ☔ View full report in Codecov by Sentry. |
| @@ -337,7 +337,7 @@ def index_get_status( | |||
| logger.error("Failed to get index status with error: %s", e) | |||
| raise types.AVSServerError(rpc_error=e) | |||
|
|
|||
| return self._respond_index_get_status(response) | |||
| return types.IndexStatusResponse.from_proto_response(response) | |||
There was a problem hiding this comment.
I think it makes more sense to have a separate helper method that creates an IndexStatusResponse from the proto response rather than a method on IndexStatusResponse. I think this because the method will be exposed to the user but the user is not concerned with how we create the object from a proto response.
| @@ -349,7 +350,7 @@ async def index_get_status( | |||
| logger.error("Failed to get index status with error: %s", e) | |||
| raise types.AVSServerError(rpc_error=e) | |||
|
|
|||
| return self._respond_index_get_status(response) | |||
| return IndexStatusResponse.from_proto_response(response) | |||
There was a problem hiding this comment.
I think it makes more sense to have a separate helper method that creates an IndexStatusResponse from the proto response rather than a method on IndexStatusResponse. I think this because the method will be exposed to the user but the user is not concerned with how we create the object from a proto response.
| field: str, | ||
| sets: str, | ||
| hnsw_params: HnswParams, | ||
| storage: IndexStorage, | ||
| storage: Optional[IndexStorage] = None, |
There was a problem hiding this comment.
Why was this change made? Because all the fields of indexStorage are optional? Are you sure that means passing None as IndexStorage works? I think a test case for this would be good.
| def from_proto_response(response: 'index_pb2.IndexStatusResponse') -> 'IndexStatusResponse': | ||
| """ | ||
| Converts a protobuf IndexStatusResponse into an IndexStatusResponse object. | ||
|
|
||
| Parameters: | ||
| ----------- | ||
| response : index_pb2.IndexStatusResponse | ||
| A protobuf IndexStatusResponse object. | ||
|
|
||
| Returns: | ||
| -------- | ||
| IndexStatusResponse | ||
| An instance of IndexStatusResponse with the values from the protobuf message. | ||
| """ | ||
| result = IndexStatusResponse() | ||
| result.unmerged_record_count = response.unmergedRecordCount | ||
| result.index_healer_vector_records_indexed = response.indexHealerVectorRecordsIndexed | ||
| result.index_healer_vertices_valid = response.indexHealerVerticesValid | ||
| return result |
There was a problem hiding this comment.
In the client these conversions are done by helper methods in conversions.py. Take for example fromIndexDefintion. I think this should be moved there.
https://aerospike.atlassian.net/browse/VEC-398