[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

--- Comment #9 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 340201
  --> https://bugs.webkit.org/attachment.cgi?id=340201
Patch

Looks almost ready to go for me.
A couple of neats and some questions below.
Btw, I created a test bugzilla entry on https://bugs.webkit.org/show_bug.cgi?id=185610.
It worked like a charm.

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

> Tools/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

That seems really nice.
I wonder whether we should provide the possibility to just create a remote branch but not create the PR right away.
The main advantage I see is that in that case, you do not have to provide the OAuth token.
Maybe, if the OAuth token is not provided, we could still create the remote branch?

> Tools/Scripts/webkitpy/tool/steps/wptchangeexport.py:1
> +# Copyright (C) 2018 Apple Inc. All rights reserved.

I think you have the right to put your own copyright.

> Tools/Scripts/webkitpy/tool/steps/wptchangeexport.py:13
> +#     * Neither the name of Google Inc. nor the names of its

Probably Apple Inc. here instead.
Also we now use a two clause license like in test_exporter.py.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:128
> +    def _ensure_username_and_token(self, options):

Can we add a FIXME to use the keychain?

> Tools/Scripts/webkitpy/w3c/test_exporter.py:362
> +    parser.add_argument('-i', '--interactive', dest='interactive_mode', action='store_true', default=False, help='Prompts the user for their github credentials and asks for confirmation before exporting the changes.')

Would it make sense to set the default value to true, or to add a no-interactive mode?

I also wonder whether it would make sense to ask user whether it is fine to store the credentials.
I think we might want to do that once we will be using the key chain.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:395
> +    host = host or Host()

Could be inlined maybe.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:402
> +        _log.info('No changes to upstream. Exiting...')

This is only style but we usually do early return.
Something like:
if not wpt_patch_generator.has_wpt_changes():
    if not silence_noop:
        log.info('No changes to upstream. Exiting...')
    return;

test_exporter = ...

Maybe silent_noop should be inverted to be something like log_if_nowptchange.

-- 
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/20180514/5ff2330f/attachment.html>