[webkit-reviews] review granted: [Bug 211707] Add Slack-aware WebKitBot implementation : [Attachment 402711] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 211707: Add Slack-aware WebKitBot implementation
https://bugs.webkit.org/show_bug.cgi?id=211707

Attachment 402711: Patch

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




--- Comment #15 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 402711
  --> https://bugs.webkit.org/attachment.cgi?id=402711
Patch

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

r=me, awesome work!

> Tools/ChangeLog:50
> +	   * WebKitBot/tests/resources/HaveRadarAndBugzilla.json: Added.
> +	   * WebKitBot/tests/resources/HavingBugzilla.json: Added.
> +	   * WebKitBot/tests/resources/NoRadarAndBugzilla.json: Added.

NIT: not sure why, but these are showing as binary files in this diff ��

> Tools/WebKitBot/ReadMe.md:21
>
+webkitWorkingDirectory="/path/to/WebKit/repository/using/used/by/revert/comman
d"

NIT: "/using/used/" :(

> Tools/WebKitBot/src/AsyncTaskQueue.mjs:47
> +	   return await this._postTask(task);

Given that this function is already `async`, do we need this `await`?  IIRC, if
`_postTask` returns a `Promise`, then it will get "forwarded" to the result of
`post`, thus avoiding an extra microtask.

> Tools/WebKitBot/src/AsyncTaskQueue.mjs:54
> +	   return await this._postTask(task);

Ditto (:47)

> Tools/WebKitBot/src/AsyncTaskQueue.mjs:68
> +		   reject

Style: missing trailing comma

> Tools/WebKitBot/src/AsyncTaskQueue.mjs:76
> +	   this.conditionPromise = new Promise((resolve) => {

this should probably be "private"

> Tools/WebKitBot/src/AsyncTaskQueue.mjs:77
> +	       this.resolve = resolve;

this should probably be "private", and could probably use a better name like
`_conditionPromiseResolve`

> Tools/WebKitBot/src/Commit.mjs:47
> +	       change = replaceAll(entry.fullName, nameWithNicks, change);

NIT: do we really need a package for this ��

> Tools/WebKitBot/src/Commit.mjs:100
> +	   if (this._bugzilla)
> +	       this._bugzilla = Number.parseInt(this._bugzilla, 10);
> +	   else
> +	       this._bugzilla = null;

NIT: you could use a ternary

> Tools/WebKitBot/src/Commit.mjs:104
> +	   if (this._radar)
> +	       this._radar = Number.parseInt(this._radar, 10);
> +	   else
> +	       this._radar = null;

Ditto (:97)

> Tools/WebKitBot/src/WKR.mjs:41
> +    await new Promise((resolve) => setTimeout(resolve, milliseconds));

Ditto (AsyncTaskQueue.mjs:47)

> Tools/WebKitBot/src/WebKitBot.mjs:93
> +	   if (firstCharacterOfReason === "'" || firstCharacterOfReason ===
"\"") {

Should we also include "`"?

> Tools/WebKitBot/src/WebKitBot.mjs:141
> +	       usage: "revert SVN_REVISION [SVN_REVISIONS] REASON\ne.g. `revert
260220 Ensure it is working after refactoring`\n`revert 260220,260221 Ensure it
is working after refactoring`",

NIT: for readability, you could use actual newlines in a template string
instead of "\n"

> Tools/WebKitBot/src/WebKitBot.mjs:148
> +	       usage: "dry-revert SVN_REVISION [SVN_REVISIONS] REASON\ne.g.
`dry-revert 260220 Ensure it is working after refactoring`\n`dry-revert
260220,260221 Ensure it is working after refactoring`",

Ditto (:141)

> Tools/WebKitBot/src/WebKitBot.mjs:199
> +    async revertCommand(event, command, args)

NIT: I feel like most of these methods should be "private"

> Tools/WebKitBot/src/WebKitBot.mjs:204
> +			await this._web.chat.postMessage({

Style: indentation

> Tools/WebKitBot/src/WebKitBot.mjs:216
> +		       text: `<@${event.user}> Preparing revert for
${escapeForSlackText(revisions.map((revision) =>
`https://trac.webkit.org/r${revision}`).join(" "))} ...`,

Are you able to use HTML/markdown formatting?  It would be cool to just show
`r######` but have it be linkified ��

> Tools/WebKitBot/src/WebKitBot.mjs:236
> +	       return;
> +	   }
> +	   await this._web.chat.postMessage({

Style: I'd add a newline after the `}` since there is a `return`

> Tools/WebKitBot/src/WebKitBot.mjs:247
> +			await this._web.chat.postMessage({

Style: indentation

> Tools/WebKitBot/src/WebKitBot.mjs:299
> +		       text: `<@${event.user}>
"${escapeForSlackText(commandName)}":
${escapeForSlackText(operation.description)}\nUsage:
${escapeForSlackText(operation.usage)}`,

Ditto (:141)

> Tools/WebKitBot/src/WebKitBot.mjs:386
> +		       "create-revert",

Style: indentation

> Tools/WebKitBot/tests/Commit.test.mjs:42
> +    expect(commit.patchBy).toBe(null);
> +    expect(commit.revert).toBe(null);

Can we add tests for these too?