[Webkit-unassigned] [Bug 184914] Export changes to web-platform-test as part of the webkit-patch upload workflow

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=184914

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #338734|review?                     |review-
              Flags|                            |

--- 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/system/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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180426/a74c9fb7/attachment.html>