Bug 48784 – Teach check-webkit-style how to accept a list of files to diff on the command line

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Teach check-webkit-style how to accept a list of files to diff on the command line

[Adam Barth]

Created attachment 72561 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 72561 [details]
Patch

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

Looks sane otherwise.  I believe your understanding of python's None argument convention is mistaken.  I feel like we have this argument often. :)

> WebKitTools/Scripts/check-webkit-style:118
> +        changed_files = paths if options.diff_files else []

I think you want else None here.  [] seems wrong.

> WebKitTools/Scripts/webkitpy/style_references.py:72
> +    def create_patch(self, git_commit, changed_files=[]):

You want None here. :)

[Tony Chang]

Comment on attachment 72561 [details]
Patch

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

>> WebKitTools/Scripts/check-webkit-style:118
>> +        changed_files = paths if options.diff_files else []
> 
> I think you want else None here.  [] seems wrong.

[] seems to be ok here (it's reallocated each time it's run), but Eric is right that the default function param should be None.

[Adam Barth]

Created attachment 72569 [details]
Patch for landing

[Adam Barth]

Committed r71064: <http://trac.webkit.org/changeset/71064>

[WebKit Review Bot]

http://trac.webkit.org/changeset/71064 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
accessibility/aria-activedescendant-crash.html
accessibility/aria-checkbox-text.html
accessibility/aria-hidden-update.html
accessibility/contenteditable-hidden-div.html
accessibility/crash-with-noelement-selectbox.html
accessibility/crashing-a-tag-in-map.html
accessibility/document-attributes.html
accessibility/first-letter-text-transform-causes-crash.html
accessibility/hang-in-isignored.html
accessibility/nested-layout-crash.html
accessibility/nochildren-elements.html
accessibility/non-data-table-cell-title-ui-element.html
accessibility/non-native-image-crash.html
accessibility/radio-button-checkbox-size.html
accessibility/removed-anonymous-block-child-causes-crash.html
accessibility/removed-continuation-element-causes-crash.html
accessibility/table-modification-crash.html
accessibility/table-nofirstbody.html
accessibility/table-notbody.html
accessibility/table-with-empty-thead-causes-crash.html

[Csaba Osztrogonác]

(In reply to comment #3)
> (From update of attachment 72561 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72561&action=review
> 
> >> WebKitTools/Scripts/check-webkit-style:118
> >> +        changed_files = paths if options.diff_files else []
> > 
> > I think you want else None here.  [] seems wrong.
> 
> [] seems to be ok here (it's reallocated each time it's run), but Eric is right that the default function param should be None.

After this patch, check-webkit-style doesn't work for me.
But it works with [] instead of None.


$ Tools/Scripts/check-webkit-style
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 134, in <module>
    main()
  File "Tools/Scripts/check-webkit-style", line 119, in main
    patch = checkout.create_patch(options.git_commit, changed_files=changed_files)
  File "/home/oszi/WebKit/Tools/Scripts/webkitpy/style_references.py", line 73, in create_patch
    return self._scm.create_patch(git_commit, changed_files=changed_files)
  File "/home/oszi/WebKit/Tools/Scripts/webkitpy/common/checkout/scm.py", line 748, in create_patch
    return self.run(['git', 'diff', '--binary', "--no-ext-diff", "--full-index", "-M", self.merge_base(git_commit), "--"] + changed_files, decode_output=False, cwd=self.checkout_root)
TypeError: can only concatenate list (not "NoneType") to list

[Csaba Osztrogonác]

Comment on attachment 72569 [details]
Patch for landing

remove cq+, because it was landed

[Csaba Osztrogonác]

sorry, it was false positive alarm. https://bugs.webkit.org/show_bug.cgi?id=52261 is the culprit.