[webkit-reviews] review denied: [Bug 182036] Create BuildbotBuildEntry in Buildbot syncer in Buildbot 0.9 format : [Attachment 332565] Proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 30 00:17:35 PST 2018
Ryosuke Niwa <rniwa at webkit.org> has denied Aakash Jain
<aakash_jain at apple.com>'s request for review:
Bug 182036: Create BuildbotBuildEntry in Buildbot syncer in Buildbot 0.9 format
https://bugs.webkit.org/show_bug.cgi?id=182036
Attachment 332565: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=332565&action=review
--- Comment #5 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 332565
--> https://bugs.webkit.org/attachment.cgi?id=332565
Proposed patch
View in context: https://bugs.webkit.org/attachment.cgi?id=332565&action=review
> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:38
> + buildRequestStatusIfUpdateIsNeeded(request)
We shouldn't be duplicating the code for this function.
r- due to this code duplication.
> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:68
> + this._isPending = false;
> + if ('claimed' in rawData)
> + this._isPending = !rawData['claimed'];
These three lines of code should just be: this._isPending = 'claimed' in
rawData && !rawData['claimed'].
> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:75
> + if (!('properties' in rawData))
> + return;
> + if (rawData.properties.workername)
> + this._workerName = rawData.properties.workername[0];
These lines code should simply be: this._workerName = rawData['properties'] &&
rawData['properties']['workername'] ? rawData['properties']['workername'][0] :
null.
But is it really guaranteed that worker name always contain exactly one item?
> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:78
> + syncer() { return this._syncer; }
Instead of duplicating all the code, make one inherit from another.
> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:271
> + urlForPendingBuild(buildrequestid) { return
this._remote.url(`/#/buildrequests/${buildrequestid}`); }
buildrequestid should be camelCase: buildRequestId
More information about the webkit-reviews
mailing list