-
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
Fixes for SSL certs, HTTP authentication, and consistent list types #26
base: main
Are you sure you want to change the base?
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.
I see that this is in draft, but I have a few comments while the code is still in active development.
list_type = str() | ||
for j in input_list: | ||
if type(j) in (str, bool, type(None)): | ||
list_type = str() | ||
break | ||
elif type(j) is float: | ||
list_type = float() | ||
elif type(j) is int and type(list_type) is not float: | ||
list_type = int() | ||
return list(map(type(list_type), input_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.
This use of str()
, et al. is not very Pythonic, as far as I'm aware. That is, rather than creating a default instance of each type and then determining their respective types at the end, just set list_type
directly to the appropriate type:
list_type = str
for j in input_list:
if type(j) in (str, bool, type(None)):
list_type = str
break
elif type(j) is float:
list_type = float
elif type(j) is int:
if list_type is not float:
list_type = int
else:
raise TypeError(f"Cannot handle a value of type {type(j)}")
return list(map(list_type, input_list))
Also, this code catches the edge case where the type is unexpected.
|
||
def convert_to_supported_type(value) -> typing.Dict: |
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 type hints on this signature look wrong: it should accept an Any
and return an Any
.
result = {} | ||
for k in value: | ||
result[convert_to_supported_type(k)] = convert_to_supported_type(value[k]) | ||
return result |
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.
More Pythonic would be:
result = {}
for k, v in value.items():
result[convert_to_supported_type(k)] = convert_to_supported_type(v)
return result
or, even better,
return {
convert_to_supported_type(k): convert_to_supported_type(v)
for k, v in value.items()
}
else: | ||
print("Unknown type", type_of_val, "with val", str(value)) |
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.
You don't need the str()
-- print()
does that implicitly.
if params.username: | ||
opensearch = OpenSearch( | ||
hosts=params.url, basic_auth=[params.username, params.password] | ||
hosts=params.url, | ||
http_auth=(params.username, params.password), | ||
verify_certs=params.tls_verify, | ||
) | ||
# Support for servers that don't require authentication | ||
else: | ||
opensearch = OpenSearch(hosts=params.url) | ||
resp = opensearch.index(index=params.index, body=params.data) | ||
opensearch = OpenSearch( | ||
hosts=params.url, | ||
verify_certs=params.tls_verify, | ||
) |
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.
You can also do this like this:
kwargs = {"hosts": params.url, "verify_certs": params.tls_verify}
if params.username:
kwargs["http_auth"] = (params.username, params.password)
opensearch = OpenSearch(**kwargs)
def validate_list_items_homogeous_type(t, input_list): | ||
if len(input_list) == 0: | ||
return # no problem with an empty 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.
You don't actually have a test case for a zero-length list -- I recommend either adding one or removing this otherwise useless code.
new_list = [] | ||
for i in value: | ||
new_list.append(convert_to_supported_type(i)) |
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.
Perhaps,
new_list = [convert_to_supported_type(v) for v in value]
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.
Two more comments on your update.
def convert_to_homogenous_list(input_list: list): | ||
# To make all types in list homogeneous, we upconvert them | ||
# to the least commom type. |
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.
Return type hint? Also, replace "commom" with "common" (although, I'm not sure that I would consider str
to be "the least common" type...).
if type(j) is dict: | ||
list_type = dict() | ||
break |
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 don't think that dict
is a type to which you can trivially promote scalars (and I suspect that promoting a list
won't produce the result that you expect):
>>> list(map(dict, [1, "2", [3,4], {5:6}]))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: 'int' object is not iterable
Changes introduced with this PR
By contributing to this repository, I agree to the contribution guidelines.