[webkit-reviews] review granted: [Bug 222897] [perf dashboard] Add commit revision label support : [Attachment 424437] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 26 21:42:14 PDT 2021
Ryosuke Niwa <rniwa at webkit.org> has granted Zhifei Fang
<zhifei_fang at apple.com>'s request for review:
Bug 222897: [perf dashboard] Add commit revision label support
https://bugs.webkit.org/show_bug.cgi?id=222897
Attachment 424437: Patch
https://bugs.webkit.org/attachment.cgi?id=424437&action=review
--- Comment #53 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 424437
--> https://bugs.webkit.org/attachment.cgi?id=424437
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=424437&action=review
> Websites/perf.webkit.org/public/v3/models/commit-log.js:61
> }
Like I pointed out earlier, we should return null explicitly here.
> Websites/perf.webkit.org/public/v3/models/commit-log.js:65
> + let formattedRevision = (() => {
Nit: use const. Two spaces after =
> Websites/perf.webkit.org/public/v3/models/commit-log.js:105
> + return `${previousCommit.label()}${separator}${this.label()}`;
Actually, this is basically same as ${formattedFrom}${separator}${formattedTo}
above.
So maybe it's better to do something like this:
const label = ((fromMatch, toMatch) => {
const separator = repositoryType == 'git' ? '..' : (repositoryType
== 'svn' ? '-' : ' - ');
const revisionRange =
`${CommitLog._formatttedRevision(fromRevision)}${separator}${CommitLog._formatt
tedRevision(toRevision);}`;
if (fromMatch && toMatch) {
console.assert(fromMatch.groups.branch ==
toMatch.groups.branch);
return
`${fromMatch.groups.number}-${toMatch.groups.number}@${fromMatch.groups.branch}
(${revisionRange})`;
}
if (fromMatch || toMatch)
return `${previousCommit.label()} - ${this.label()}`;
return revisionRange;
})(identifierPattern.exec(previousCommit.revisionIdentifier()),
identifierPattern.exec(this.revisionIdentifier()));
More information about the webkit-reviews
mailing list