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
Simplify get_legend_handler method #6639
Simplify get_legend_handler method #6639
Conversation
It is not clear for me why there was no check for unhashble legend_handler_keys = list(six.iterkeys(legend_handler_map))
if orig_handle in legend_handler_keys: Should I add such check or close pull request? |
It is side-stepping having to do a check by using a simpler data structure. The I would revert the first set of changes, add a comment as to why it is that way, and leave the second half. Adding a test to the test suite that exercises the pathological input would be good too. |
Yes, I understand this, but here we have a linear search in a dictionary by creating temporary list of dictionary keys and this list lives only withing the function. def get_legend_handler(legend_handler_map, orig_handle):
try:
hash(orig_handle)
except TypeError:
return None
... |
else: | ||
try: | ||
hash(orig_handle) | ||
except TypeError: |
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 is different behavior, If it is not hash-able, then we need to walk it's MRO to see if we have a handle for any of it's classes.
This is apparently not tested well enough.
7d610d1
to
55adc49
Compare
I have updated PR as per your suggestion and squashed multiple commits |
I have simplified the
get_legend_handler
method. There is no need to create list from keys oflegend_handler_map
and no need in two dictionary lookups (if key in dict: return dict[key]
).Loop can be optimized even more, but will result in a code bloat, which does not worth of saving few cpu cycles.