-
Notifications
You must be signed in to change notification settings - Fork 10
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
Bulk Scan Updates #94
base: master
Are you sure you want to change the base?
Conversation
@ragini7913 It looks like bulk_scan.html is missing, can you please upload that and address the lint errors? |
microsetta_admin/server.py
Outdated
code = [random.choice(hex) for i in range(6)] | ||
color_code = ['#' + ''.join(code)] | ||
while color_code[0] in dict: | ||
code = [random.choice(hex) for i in range(6)] | ||
color_code = ['#' + ''.join(code)] | ||
dict[obj["values"][val]] = color_code[0] |
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 issues here:
- This code is essentially repeated twice below. Please decompose it into a function that only handles the task of generating a dictionary of colors.
- The code can produce colors that are very similar, which somewhat defeats the purpose of color-coding. Can you consider alternatives that would do a better job of ensuring visual clarity?
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.
Created a function to generate random color code. The logic to generate color codes is adjusted so that produced colors are not very similar. The code is further simplified
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.
microsetta_admin/server.py
Outdated
while color_code in dict: | ||
color_code = _get_color_code() | ||
dict[obj["values"][val]] = color_code | ||
|
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 the function return an error if it's called with an invalid criteria?
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.
In case the function is called with an invalid criteria, it will return an empty dictionary.
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.
It looks like there's nothing checking that between the function and the UI. Should there be a check and explicit exception if the result is an empty dictionary, rather than letting the UI silently fail?
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.
In case of invalid criteria, visualization must show all samples to "None" legend for provided criteria. I have updated code to return the legend dictionary to at least return "None" as value for every sorting criteria.
color_code = _get_color_code() | ||
|
||
if color_code is not None: | ||
while color_code in dict.values(): |
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.
It looks like this has the potential to go into an infinite loop if the supply of defined colors is exhausted. Can that be handled more gracefully?
microsetta_admin/server.py
Outdated
while color_code in dict: | ||
color_code = _get_color_code() | ||
dict[obj["values"][val]] = color_code | ||
|
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.
It looks like there's nothing checking that between the function and the UI. Should there be a check and explicit exception if the result is an empty dictionary, rather than letting the UI silently fail?
Hi @raginirai553 please see the below comments/issues:
|
microsetta_admin/server.py
Outdated
@@ -964,11 +966,13 @@ def _post_bulk_scan(): | |||
obj = {} | |||
obj["rack_id"] = rec[6] | |||
if math.isnan(rec[3]): | |||
obj['location_col'] = 'None' | |||
error_msg = "Error: Empty column in row number" + rowCnt | |||
break | |||
else: | |||
obj["location_col"] = str(int(rec[3])) | |||
if math.isnan(rec[4]): |
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 isnan check needs to be removed, rec[4] will never be a number. Also, the error message strategy as written doesn't work - when it hits the isnan block, it throws an internal server error, meaning the user will never see the error.
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.
@raginirai553 There are a couple of significant issues with the Bulk Scan function that I'm experiencing in testing:
Are you experiencing either of these issues in your local development environment? |
No description provided.