-
Notifications
You must be signed in to change notification settings - Fork 2
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
Initial Commit #6
Initial Commit #6
Conversation
Organized imports
Changed private functions from two underscores to one.
Added wait for completion on delete Added smoke tests for all API's Ran black and flake8 Added Logging
Added setup files in test dir
Black format
Added fixes for github actions testing
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.
I glanced over this but I think it's too in the weeds and big of PR for me to really weigh in. Approved.
@@ -0,0 +1,2 @@ | |||
[flake8] | |||
ignore = E501 |
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.
ignore = E501 | |
max-line-length = 88 # black compatability | |
ignore = E501, E203 # black compatability |
I believe all three are needed for black compatibility
.github/workflows/unit_test.yml
Outdated
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 file is called unit_tests.yml, but it looks more like integration/e2e because you are spinning up aerospike/proximus
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.
Isn't this code auto-generated? If that's the case we might want to tell black to ignore the autogenerated files? Otherwise, we will need to ensure that black gets run on each newly generated file
|
||
|
||
class HostPort(object): | ||
def __init__(self, address: str, port: int, isTls=False): | ||
self.address = address | ||
def __init__(self, *, host: str, port: int, isTls: Optional[bool] = False) -> None: |
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.
Why only keyword arguments?
def __str__(self): | ||
bins_info = "" | ||
for key, value in self.bins.items(): | ||
if isinstance(value, list): |
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.
Should we also handle dicts?
|
||
except Exception as e: |
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.
logger?
endpoint.address, endpoint.port, endpoint.isTls | ||
) | ||
except Exception as e: | ||
warnings.warn("error creating channel: " + str(e)) |
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.
logger
endpoints: vector_db_pb2.ServerEndpointList) -> grpc.Channel: | ||
def _create_channel_from_server_endpoint_list( | ||
self, endpoints: vector_db_pb2.ServerEndpointList | ||
) -> grpc.aio.Channel: | ||
# TODO: Create channel with all endpoints |
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.
Is the TODO completed by looping through all endpoints?
host = re.sub(r'%.*', '', host) | ||
return grpc.insecure_channel(f'{host}:{port}') | ||
host = re.sub(r"%.*", "", host) | ||
return grpc.aio.insecure_channel(f"{host}:{port}") |
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.
Again, I am surprised that this does not need an await
. Can you help me understand that if you know why?
bin_selector = self.__getBinSelector(bin_names) | ||
self.__channelProvider.get_channel() | ||
) | ||
key = _get_key(key, set_name, namespace) |
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.
I find the order of these args to be unsettling :)
No description provided.