[webkit-reviews] review granted: [Bug 91890] nrwt: never finds binaries in the 'out' dir on chromium win : [Attachment 153594] fix changelog

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 91890: nrwt: never finds binaries in the 'out' dir on chromium win
https://bugs.webkit.org/show_bug.cgi?id=91890

Attachment 153594: fix changelog
https://bugs.webkit.org/attachment.cgi?id=153594&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=153594&action=review


>>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:83
>>>		 return filesystem.join(build_directory, *comps)
>> 
>> How does this work without the * in front of comps?
> 
> taking a tuple and putting the * in front of it basically expands the
argument into a varargs list.

Ah, right, because we're passing it to a function.

>>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:393
>>> +	     return self._static_build_path(self._filesystem,
self.get_option('build_directory'), self.path_from_chromium_base(),
self.path_from_webkit_base(), configuration=configuration, comps=comps)
>> 
>> Why are configuration and comps named? Shouldn't comps be *comps?
> 
> no particular reason for the names; I actually thought they were all named
but I obviously didn't check closely. cf. above for comps vs *comps. I don't
like to use the vararg positional parameters in a function that has so many
other parameters as well; that said, the change was done originally because of
the interaction with the kwargs (since configuration came after comps), so I
could back it out now.
> 
> let me know which you prefer.

I would remove the explicit naming since they are not optional parameters and
we don't normally name non-optional params.