[webkit-reviews] review denied: [Bug 184914] Export changes to web-platform-test as part of the webkit-patch upload workflow : [Attachment 338734] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Brendan McLoughlin
<brendan at bocoup.com>'s request for review:
Bug 184914: Export changes to web-platform-test as part of the webkit-patch
upload workflow
https://bugs.webkit.org/show_bug.cgi?id=184914

Attachment 338734: Patch

https://bugs.webkit.org/attachment.cgi?id=338734&action=review




--- Comment #4 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 338734
  --> https://bugs.webkit.org/attachment.cgi?id=338734
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338734&action=review

> Tools/Scripts/webkitpy/tool/commands/upload.py:289
> +	   steps.W3CTestExport,

We should rename this to WPTChangeExport since this is all about
web-platform-tests,
and doesn't apply to other W3C test repositories.

> Tools/Scripts/webkitpy/tool/steps/w3ctestexport.py:46
> +	   args = []

The initial declaration of this variable should contain --bug, and other
options that area always specified
instead of adding each one separately down below.

> Tools/Scripts/webkitpy/tool/steps/w3ctestexport.py:52
> +	   bug_id = state.get("bug_id")
> +	   if not bug_id:

Please check this condition first.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:53
> +    """the W3cChangeset class is responsible for deteching changes in
> +    the web-platform-test directory and generating patches exporting
> +    those changes.
> +    """

We don't add these comments. If we find ourselves needing to a comment like
this,
it's a good indication that the class name isn't descriptive enough. Please
remove the comment.
I would call this class WebPlatformTestPatchGenerator instead.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:67
> +	   git_commit = "HEAD...." if not self._options.git_commit else
self._options.git_commit + "~1.." + self._options.git_commit
> +	   patch_data = self._host.scm().create_patch(git_commit,
[WEBKIT_WPT_DIR])

This assumes scm is a git. This is simply not true for many WebKit
contributors.
r- because of this. Use scm.create_patch instead.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:73
> +	   patch_file = './patch.temp.' + str(time.clock())

Please create a temporary file in a temporary directory via
self._filesystem.open_binary_tempfile
r- because of this as well.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:139
> +	   """
> +	   Ask the user to provide a username and token if the
> +	   --interactive flag is passed and username and token
> +	   are not already set.
> +	   """

This comment repeats the code below. Please remove.
In general, we don't add these kinds of "what" comment in WebKit.
We only add "why" comments.
r- due to this and the earlier class-level "what" comments.
Also, please use
https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/syst
em/user.py

> Tools/Scripts/webkitpy/w3c/test_exporter.py:142
> +	       self._username =
self._git.local_config('github.username').rstrip()

Please use
https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/net/
credentials.py
and read username/password from KeyChain as well.