Conversation
|
Do we want something like:
to raise ValueError or just return an empty generator? |
|
I think a ValueError or whatever the equivalent error you'd get from |
|
That doesn't throw an error either, it also just returns an empty generator. So throwing an error in this case might actually break the API. |
|
So other than the uncertainty surrounding raising an error or not, this PR can be reviewed 👍 |
|
@jonmmease can you give this PR a sanity-check, implementation-wise, please? |
|
The behaviour seems like what I want :) |
jonmmease
left a comment
jonmmease
left a comment
There was a problem hiding this comment.
Functionality and tests look good!
I left a few comments/suggestions on design.
| """ | ||
|
|
||
| # if selector is not an int, we call it on each trace to test it for selection | ||
| if type(selector) != type(int()): |
There was a problem hiding this comment.
Unless there's a reason not to, I'd recommend if not isinstance(selector, int): for consistency
There was a problem hiding this comment.
done, along with all the other similar issues flagged by flake8
| return True | ||
| # If selector is a string then put it at the 'type' key of a dictionary | ||
| # to select objects where "type":selector | ||
| if type(selector) == type(str()): |
There was a problem hiding this comment.
Unless there's a reason not to, I'd recommend if not isinstance(selector, six.string_types): for consistency
| _natural_sort_strings(list(self.layout)), | ||
| ) | ||
| layout_objs = [self.layout[k] for k in layout_keys] | ||
| return _generator(self._filter_by_selector(layout_objs, [], selector)) |
There was a problem hiding this comment.
This reduction over filters approach is clever, but I think it's a bit more opaque than the current paradigm.
Also, before it returns anything this method traverses the full layout and creates several fully instantiated lists and then converts the final list to a generator. While performance probably isn't a major factor here, the original design avoids the creation of any of these lists, and it returns results one at a time as matches are found.
Refactoring stuff into functions is fine, but I'd prefer to keep the original overall structure of:
layout_keys = _natural_sort_strings(list(self.layout))
for k in layout_keys:
# filter and continue on stuff that doesn't pass filter
yield self.layout[k]| and (cur[0]["y"][1] == cur[1]), | ||
| zip(trs, [10, 20]), | ||
| True, | ||
| ) |
There was a problem hiding this comment.
IMO this check would be a lot clearer using a comprehension inside all instead reduce.
Something like
all(trace["type"] == "bar" and trace["y"][1] == y
for trace, y in zip(trc, [10, 20]))
If selector is string, transformed to dict(type=selector), if selector is int, indexes the resulting list of objects filtered down by row, col and secondary y.
if selector is string, it is converted to dict(type=selector)
e.g., select_xaxes uses this selector note: need to test new selector argument that is integer or string
string doesn't make sense in this case as they don't have a "type" key.
e.g., select_xaxes
nicolaskruchten
force-pushed
the
selector-accept-int-string
branch
from
d247ab4 to
e33e10b
Compare
3 participants
closes #2891