[webkit-reviews] review denied: [Bug 111513] Create a script to import W3C tests : [Attachment 194324] Now ready for review

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Rebecca Hauck
<rhauck at adobe.com>'s request for review:
Bug 111513: Create a script to import W3C tests
https://bugs.webkit.org/show_bug.cgi?id=111513

Attachment 194324: Now ready for review
https://bugs.webkit.org/attachment.cgi?id=194324&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=194324&action=review


Okay, as per our discussion over IM/IRC earlier today, I think it makes sense
to land this and iterate as soon as we can, even if the code isn't ideal. The
scripts shouldn't break anything else or affect anyone's checkout in
particular.

Please address as many of the stylistic nits as you can (especially the
copyright and changelog info). Don't worry about cleaning up the tests too
much. Re-post another patch just so we can be sure it'll still apply against
the tree and we should be able to land something soon.

Thanks for bearing with me during the long delay!

> Tools/Scripts/webkitpy/w3c/test_converter.py:125
> +	   @return True if changes were made

We don't usually use javadoc-style @param/@return notation for parameters. We
would probably do something more like:

def convert_testharness_paths(self, doc, new_path):
   """Update links to testharness.js in the BeautifulSoup |doc| to point to the
copy in |new_path|. Returns whether the document was modified."""

I.e., you can get a lot terser. A "proper" pythonic docstring has a one-line
summary, then a blank line, then the rest of the string, but we don't tend to
stick to this that carefully and favor shorter docstrings (or none at all)
where they're not really needed.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:3
> +# Copyright (C) 2010 Apple Inc. All rights reserved.

update copyright here as well.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:77
> +"""

There's a lot of boilerplate text in these tests, and it's hard to tell which
parts of the string are relevant and change from test to test. We should try to
wrap the test html in a generator function or something.

> Tools/Scripts/webkitpy/w3c/test_importer.py:3
> +# Copyright (C) 2010 Apple Inc. All rights reserved.

update the copyright.

> Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:3
> +# Copyright (C) 2010 Apple Inc. All rights reserved.

update.

> Tools/Scripts/webkitpy/w3c/test_parser.py:3
> +# Copyright (C) 2010 Apple Inc. All rights reserved.

update.

> Tools/Scripts/webkitpy/w3c/test_parser_unittest.py:3
> +# Copyright (C) 2010 Apple Inc. All rights reserved.

:).

> Tools/Scripts/webkitpy/w3c/test_parser_unittest.py:98
> +"""

same comments about repeat blocks of text.