[webkit-reviews] review denied: [Bug 60314] Add Bucket class to webkitpy for upcoming new baseline tool : [Attachment 92495] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied James Kozianski <koz at chromium.org>'s
request for review:
Bug 60314: Add Bucket class to webkitpy for upcoming new baseline tool
https://bugs.webkit.org/show_bug.cgi?id=60314

Attachment 92495: Patch
https://bugs.webkit.org/attachment.cgi?id=92495&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92495&action=review

I got kinda lost about half way in.  I can make a second attempt at reading,
but I've already left you plenty of comments below.  I'll mark this r- while
you get a chance to reply to them.  Feel free to mark it r? again if you feel
the patch should still be reviewed as is!

> Tools/Scripts/webkitpy/common/indented_logger.py:27
> +class IndentedLogger(object):
> +    """Logger that prints indented messages"""

Nice.  We don't already have one of these?  We do indented logging for the help
messages for webkit-patch help iirc.

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:28
> +class Bucket(object):

Bucket is a pretty generic name.  TestBucket?  PlatformBucket?	How is a bucket
different from a set?  Seems it has some extra state.

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:33
> +	   self._children = []

bucket sounds like a set concept.  Do the children have order?

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:37
> +	   # Aggregate buckets don't get results directly from buildbot.
> +	   self._is_aggregate = False

Should AggragateTestBucket be a separate class?

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:40
> +    def _on_changed(self):
> +	   for child in self._children:

What changed?

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:45
> +	   if len(results) == 0:
> +	       return True

if not results:  would be how I would have written that.

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:47
> +	   return all(map(lambda x: x.contents() == contents, results))

I think you can use operator._equal and sum() to do this.

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:49
> +    def dump(self, logger=IndentedLogger()):

foo=Foo() is dangerous because there will be a single instance of Foo() created
for the entire process.  Normally it's better to do:

foo=None in the args and then:

foo = foo or Foo() in the body.

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:63
> +	   """Buckets that don't map to a specific platform are 'aggregate'

I'm still not sure what  bucket is... :)

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:70
> +    def append(self, child):

What are our children?	More buckets?  results?

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:72
> +	   self._on_changed()

I would probably have called this _children_changed()

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:75
> +	   if self._result is not None:

is "if self._result" safe here?  I only bother to compare to None directly when
it isn't... but I may just be lazy. :)

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:77
> +	   return self._implicit_result

I'm not sure I understand yet what the difference between resutlt and implicit
result is.

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:79
> +    def set_result(self, result):

Seems like we might want an _internal_set_result which does the actual setting
and the on_changed call.

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:82
> +	       self._on_changed()

I see, so this is not just a _children_changed concept.  maybe it should be
given some sort of explicit _update_whatever() name to explain what it does
instead of when it shoudl be called.

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:90
> +	   else:

No need for else after raise or return.

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:110
> +    def _get_common_child_result(self, results):

webkit style tends to avoid _get in names.

> Tools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:111
> +	   if len(results) == 0:

Again, I would probably write if not results: but I'm a lazy typer. :)