[webkit-reviews] review denied: [Bug 179743] Add support for fetching recent builds in Buildbot 0.9 format in BuildbotSyncer : [Attachment 332004] Proposed patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Aakash Jain
<aakash_jain at apple.com>'s request for review:
Bug 179743: Add support for fetching recent builds in Buildbot 0.9 format in
BuildbotSyncer
https://bugs.webkit.org/show_bug.cgi?id=179743

Attachment 332004: Proposed patch

https://bugs.webkit.org/attachment.cgi?id=332004&action=review




--- Comment #3 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 332004
  --> https://bugs.webkit.org/attachment.cgi?id=332004
Proposed patch

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

We need tests for this. r- due to the lack of tests.

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:219
> +	       const entries = [];
> +	       for (let build of content.builds)
> +		   entries.push(new BuildbotBuildEntry(this, build));
> +	       return entries;

There is no point in manually pushing. Just do:
return content.builds.map((build) => new BuildbotBuildEntry(this, build));

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:224
> +    pathForRecentBuilds(count) { return
`/api/v2/builders/${this._builderID}/builds?` +
`limit=${count}&order=-number&property=*`; }

What why `+ ` between "builds?" and "limit="? Please concatenate them instead.