[Webkit-unassigned] [Bug 184368] Added a helper function in CommitSet which will be shared between multiple independent incoming changes.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=184368

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #337412|review?                     |review+
              Flags|                            |

--- Comment #4 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 337412
  --> https://bugs.webkit.org/attachment.cgi?id=337412
Patch

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

r=me assuming the following comments are addressed.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:149
> +    static createNameWithoutCollision(name, hasDuplicateTestGroupName)

Why don't we make this function take a set of names instead of a closure
since most of other call sites you're adding will be using a set.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:204
> +                nameParts.push(`${repository.name()}: Patch(${nameForFirstPatch}) - Patch(${nameForSecondPath})`);

We shouldn't make up a new syntax like Patch(a.patch).
Just say: "WebKit: a.patch - b.patch"
Filenames and revision/git hashes look sufficiently different that the confusion
between the two isn't really a serious practical concern as far as I can tell.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:207
> +                nameParts.push(`${repository.name()}: ${firstCommit.label()} with Patch(${nameForFirstPatch}) - ${secondCommit.label()} with Patch(${nameForSecondPath})`);

Ditto. We shouldn't be using unnecessarily verbose syntax like Patch(a.patch).
Just say: WebKit: r12345 with a.patch - r12346 with b.patch.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:217
> +            const leftRootFileDescription = uniqueInFirstCommitSet.map((rootFile) => nameGenerator(rootFile.filename())).join(', ');
> +            const rightRootFileDescription = uniqueInSecondCommitSet.map((rootFile) => nameGenerator(rootFile.filename())).join(', ');

These should firstDescription and secondDescription.
Left & right are very LTR centric way of looking at things.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:218
> +            nameParts.push(`Roots: ${leftRootFileDescription.length ? leftRootFileDescription : 'none'} - ${rightRootFileDescription.length ? rightRootFileDescription : 'none'}`);

"leftRootFileDescription || 'none'" would be sufficient here since '' is falsey.

> Websites/perf.webkit.org/unit-tests/commit-set-tests.js:334
> +        });

You need a test case where two repositories' commits differ.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180407/e8f83839/attachment-0002.html>