Conversation
I was surprised that the empty string defaulted to True, this makes it more explicit.
FelipeDefensor
left a comment
There was a problem hiding this comment.
This is mostly approved. There is just one relevant bug for which I am working on a fix. Will push it tomorrow.
It would be good to address the comments, but it is not priority.
| return ( | ||
| self.timeline.alter_levels( | ||
| self.elements_to_components(list(reversed(elements))), 1 | ||
| ) | ||
| and self._check_timeline_height() |
There was a problem hiding this comment.
I don't see why _check_timeline_height would return a bool if we didn't need to make this line work. This is conciser, but the following would be more readable, and do without the _check_timeline_height return value:
result = self.timeline.alter_levels(...)
self._check_timeline_height()
return result
There was a problem hiding this comment.
I don't see why _check_timeline_height would return a bool if we didn't need to make this line work. This is conciser, but the following would be more readable, and do without the _check_timeline_height return value:
result = self.timeline.alter_levels(...) self._check_timeline_height() return result
Because doing return a() and b() means that if a() returns False, b() doesn't run and False is returned. Still, happy to rewrite.
There was a problem hiding this comment.
Oh, that's true, but the performance gain from not evaluating b() is negligible in this case, right? In any case, we could still do:
result = self.timeline.alter_levels(...)
if result:
self._check_timeline_height()
return result
| return ( | ||
| self.timeline.group(self.elements_to_components(elements)) | ||
| and self._check_timeline_height() | ||
| ) |
|
|
||
| return True | ||
|
|
||
| def _check_timeline_height(self): |
There was a problem hiding this comment.
This does more than check. A better name would be _adjust_timeline_height.
FelipeDefensor
left a comment
There was a problem hiding this comment.
Good to go from my part. Addressing the comments is optional. Review my changes before merging.
FelipeDefensor
left a comment
There was a problem hiding this comment.
The capital letter already indicated the default on your implementation. I forgot about that convention, sorry. At least we have the option of setting a default now.
29177d3 seems to resolve #428