[webkit-reviews] review granted: [Bug 64225] nrwt: linting fixes : [Attachment 100201] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 64225: nrwt: linting fixes
https://bugs.webkit.org/show_bug.cgi?id=64225

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

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


Looks good.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:225
>	   self.msg = reason

We should have probably just passed reason as msg to the constructor.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:757
> +	   for _ in xrange(num_workers):
>	       manager_connection.post_message('stop')

Huh?  Why do we post the same message num_workers times?  Does that guarentee
that each gets the message?  I assume "stop" is the last mesage any worker will
grab?

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:-127

> -	   raise NotImplementedError

Yay!  Throwing NotImplemented from a constructor seems wrong to me. :)	I'm
glad pylint agrees.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:55
> +	   self._filesystem = None

Huh?  It's passed a filesystem later?

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:70
>	   This routine exists so that the mixin can be created and then
marshaled
>	   across into a child process."""
>	   self._port = port
> -	   self._filesystem = port._filesystem
> +	   self._filesystem = port.filesystem

I'm very confused by the existance of this safe_init method.  This smells
wrong.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:239
> -	   result = getattr(thread, 'result', None)
> +	   result = thread.result

Die evil get/has/setattr calls! Die, die!

> Tools/Scripts/webkitpy/layout_tests/port/base.py:196
> +	       _ = self._executive.run_command(['ruby', '--version'])

Um.  Just remove the _= :)

> Tools/Scripts/webkitpy/layout_tests/port/base.py:218
> +	       _ = self._executive.run_command([self._path_to_wdiff(),
'--help'])

Same here.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:275
> +	   def to_raw_bytes(string_value):
> +	       if isinstance(string_value, unicode):
> +		   return string_value.encode('utf-8')
> +	       return string_value

Ick.  This smells wrong too.  We should know what type of data we're passing
around...

> Tools/Scripts/webkitpy/layout_tests/port/base.py:525
> +    def skipped_layout_tests(self):
> +	   return []

I wonder what prompted this addition.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:951
> -	   for system in self.all_systems():
> +	   for version, architecture in self.all_systems():

much nicer!