Bug 211707 – Add Slack-aware WebKitBot implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Add Slack-aware WebKitBot implementation

[Yusuke Suzuki]

Created attachment 398988 [details]
Patch

[Yusuke Suzuki]

Created attachment 398989 [details]
Patch

[Yusuke Suzuki]

Created attachment 398995 [details]
Patch

[Yusuke Suzuki]

Created attachment 398996 [details]
Patch

[Darin Adler]

Comment on attachment 398996 [details]
Patch

I had no idea that JavaScript modules use Maciej’s initials as their file extension. I don’t think I know JavaScript well enough to review this!

[Yusuke Suzuki]

Created attachment 402457 [details]
Patch

[Yusuke Suzuki]

Created attachment 402459 [details]
Patch

[Blaze Burg]

Comment on attachment 402459 [details]
Patch

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

> Tools/WebKitBot/ReadMe.md:5
> +WKR is fetching git.webkit.org RSS feed periodically, extracting data from that, and posting them to #changes channel to replace IRC's WKR bot purpose.

Nit: I would remove "to replace..." since it won't be relevant what IRC bot used to do after this patch lands.

> Tools/WebKitBot/WKR.mjs:134
> +            throw new Error(`Canont find revision`);

Nit: Cannot

[Blaze Burg]

Comment on attachment 402459 [details]
Patch

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

This really needs some sort of test coverage to exercise the disallowed inputs, even if it's as simple as:

input: lines of text
output: dry-run output of shell commands and chat output

The fetching of messages and sending of messages / shell commands can be stubbed out.

> Tools/WebKitBot/WebKitBot.mjs:88
> +        for (let revision of extracted)

Nit: Node.js v6 and later supports destructuring.

revisions.push(...extracted)

And earlier:

revisions.push.apply(revisions, extracted)

> Tools/WebKitBot/WebKitBot.mjs:96
> +            if (reason.charAt(reason.length - 1) === firstCharacterOfReason)

Please check that this implementation supports smart quotes, as this has been a point of frustration in the past with webkitbot.

> Tools/WebKitBot/WebKitBot.mjs:101
> +    return {

Nit: this could be on one line.

> Tools/WebKitBot/WebKitBot.mjs:197
> +        // 1. Convert smart quotes to normal ASCII quotes because webkit-patch cannot accept non-ASCII text and slack may convert normal quotes to smart quotes.

EDIT: Ah, I see, this is handled at a higher level.

[Devin Rousso]

Comment on attachment 402459 [details]
Patch

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

overall i think this is great =D

lots of Style/NIT, mostly based on the Web Inspector style guide, so up to you as to whether you want to follow that for this

to echo what @Brian Burg said, tests would be nice :)

> Tools/WebKitBot/AsyncTaskQueue.mjs:30
> +        this.tasks = [];
> +        this.limit = limit;

NIT: i think these could both be "private" (prefixed with `_`)

> Tools/WebKitBot/AsyncTaskQueue.mjs:57
> +    length()

NIT: perhaps make this a `get length()`?

> Tools/WebKitBot/WKR.mjs:31
> +import fs from "fs"
> +import path from "path"
> +import RSS from "rss-parser"
> +import replaceAll from "replaceall"
> +import storage from "node-persist"
> +import axios from "axios"

Style: these should all end in `;`

> Tools/WebKitBot/WKR.mjs:42
> +    await new Promise(function (resolve) {

Style: no space between `function` and `(` if it's anonymous

> Tools/WebKitBot/WKR.mjs:56
> +        this.emails = new Map();

NIT: you can drop the `()` if there are no arguments to a constructor

> Tools/WebKitBot/WKR.mjs:58
> +        this.entries = Object.values(data);
> +        for (let [fullName, entry] of Object.entries(data)) {

NIT: rather than iterate `data` twice, perhaps make `this.entries = [];` and then `this.entries.push(entry)` in the loop

> Tools/WebKitBot/WKR.mjs:86
> +                continue;

Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

> Tools/WebKitBot/WKR.mjs:107
> +        [this.revision, change] = Commit.findAndRemove(change, /^git-svn-id: https:\/\/svn\.webkit\.org\/repository\/webkit\/trunk@(\d+) /im);
> +        [this.patchBy, change] = Commit.findAndRemove(change, /^Patch\s+by\s+(.+?)\s+on(?:\s+\d{4}-\d{2}-\d{2})?\n?/im);
> +        [this.revert, change] = Commit.findAndRemove(change, /(?:rolling out|reverting) (r?\d+(?:(?:,\s*|,?\s*and\s+)?r?\d+)*)\.?\s*/im);
> +        [this.bugzilla, change] = Commit.findAndRemove(change, /https?:\/\/(?:bugs\.webkit\.org\/show_bug\.cgi\?id=|webkit\.org\/b\/)(\d+)/im);

this is nice 😃

> Tools/WebKitBot/WKR.mjs:133
> +        if (!this.revision)

NIT: I'd add a newline before this for readability

> Tools/WebKitBot/WKR.mjs:145
> +        if (this.bugzilla)
> +            results.push(`https://webkit.org/b/${this.bugzilla}`);

Do you want to pull out radar links too? 😃
```
    [this.radar, change] = Commit.findAndRemove(change, /<rdar:\/\/(?:problem\/)?(\d+)>/im);
```

> Tools/WebKitBot/WKR.mjs:155
> +        this.web = webClient;
> +        this.auth = auth;
> +        this.revision = revision;

NIT: i think these could all be "private" (prefixed with `_`)

> Tools/WebKitBot/WKR.mjs:161
> +            "text": commit.message()

Style: no need for quotes around `text`
Style: add trailing comma

> Tools/WebKitBot/WKR.mjs:164
> +        if (!DEBUG)

Should we `console.log` the data when `DEBUG` (i.e. in an `else`)?

> Tools/WebKitBot/WKR.mjs:171
> +        console.log(`${Date.now()}: poll data`);

Should we only do this when `DEBUG`?

> Tools/WebKitBot/WKR.mjs:194
> +        console.log(`Previous Revision: ${revision}`);
> +        console.log(`Endpoint: ${process.env.slackURL}`);

Should we only do this when `DEBUG`?

> Tools/WebKitBot/WKR.mjs:201
> +        for (;;) {

Is there a difference/preference over `while (true)`?

> Tools/WebKitBot/WebKitBot.mjs:30
> +import path from "path"
> +import util from "util"
> +import { execFile, spawn } from "child_process"
> +import SlackRTMAPI from "@slack/rtm-api";
> +import AsyncTaskQueue from "./AsyncTaskQueue.mjs"

Style: these should all end in `;`

> Tools/WebKitBot/WebKitBot.mjs:49
> +        return null;

Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

> Tools/WebKitBot/WebKitBot.mjs:52
> +        return match[1];

Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

> Tools/WebKitBot/WebKitBot.mjs:65
> +            continue;

Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

> Tools/WebKitBot/WebKitBot.mjs:68
> +            return null;

Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

> Tools/WebKitBot/WebKitBot.mjs:118
> +        this.taskQueue = new AsyncTaskQueue(defaultTaskLimit);
> +        this.web = webClient;
> +        this.auth = auth;

NIT: i think these could all be "private" (prefixed with `_`)

> Tools/WebKitBot/WebKitBot.mjs:120
> +        this.commands = new Map();

NIT: you can drop the `()` if there are no arguments to a constructor

> Tools/WebKitBot/WebKitBot.mjs:135
> +            description: "Responds with pong.",

NIT: you could add a "to check if WebKitBot is alive/working" to further explain this :)

> Tools/WebKitBot/WebKitBot.mjs:159
> +            switch (event.type) {
> +            case "message": {

If you only have one `case`, why not just make this into an early-return `if (event.type !== "message") return;`?

> Tools/WebKitBot/WebKitBot.mjs:162
> +                    return;

Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

> Tools/WebKitBot/WebKitBot.mjs:181
> +        setInterval(async () => {
> +            await this.taskQueue.postOrFailWhenExceedingLimit({

Making this `async` doesn't actually do anything IIUC because `setInterval` won't wait on the result of an `async` function before scheduling the next call.  If you want the next call to be scheduled after the previous call finishes, I'd do
```
    setTimeout(async function pull() {
        await this.taskQueue.postOrFailWhenExceedingLimit({
            command: "pull",
        });

        setTimeout(pull, defaultPullPeriod);
    }, defaultPullPeriod);
```
otherwise, I'd drop the `async` and `await` as they don't really do anything :/

> Tools/WebKitBot/WebKitBot.mjs:193
> +            return null;

Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

> Tools/WebKitBot/WebKitBot.mjs:209
> +			await this.web.chat.postMessage({

Style: indentation

> Tools/WebKitBot/WebKitBot.mjs:211
> +				text: `<@${event.user}> webkit-patch only accepts an ASCII string for reason: \`${escapeForSlackText(reason)}\``

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:216
> +        console.log(revisions, reason);

Should we only do this when `DEBUG`?

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

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:230
> +                    text: `<@${event.user}> Created a revert patch https://webkit.org/b/${escapeForSlackText(bugId)}`

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:236
> +                    text: `<@${event.user}> Failed to create revert patch.`

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:243
> +            text: `<@${event.user}> Failed to parse revision and reason`

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:252
> +			await this.web.chat.postMessage({

Style: indentation

> Tools/WebKitBot/WebKitBot.mjs:254
> +				text: `<@${event.user}> webkit-patch only accepts an ASCII string for reason: \`${escapeForSlackText(reason)}\``

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:262
> +                text: `<@${event.user}> No revision is found: reason = \`${escapeForSlackText(reason)}\``

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:269
> +            text: `<@${event.user}> revisions = \`${escapeForSlackText(revisions.join(","))}\`, reason = \`${escapeForSlackText(reason)}\``

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:277
> +            text: `<@${event.user}> Preparing pulling the latest WebKit checkout.`

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:284
> +            text: `<@${event.user}> Pulled the latest checkout.`

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:292
> +            text: `<@${event.user}> pong`

Style: add trailing comma

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

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:309
> +                    text: `<@${event.user}> Unknown command "${escapeForSlackText(commandName)}"`

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:318
> +                text: `<@${event.user}> Available commands: ${escapeForSlackText(commandNames.join(", "))}\nType "help COMMAND" for help on my individual commands.`

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:327
> +            text: `<@${event.user}> ${escapeForSlackText(this.taskQueue.length())} requests in queue.`

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:333
> +        console.log("Unknown command: ", command);

Should we only do this when `DEBUG`?

> Tools/WebKitBot/WebKitBot.mjs:336
> +            text: `<@${event.user}> Unknown command "${escapeForSlackText(command)}"`

Style: add trailing comma

> Tools/WebKitBot/WebKitBot.mjs:348
> +            task.on('close', (code) => {

Style: double quotes

> Tools/WebKitBot/WebKitBot.mjs:359
> +        console.log("1. Resetting");

Should we only do this when `DEBUG`?

> Tools/WebKitBot/WebKitBot.mjs:362
> +        console.log("2. Cleaning");

Should we only do this when `DEBUG`?

> Tools/WebKitBot/WebKitBot.mjs:365
> +        console.log("3. Pulling");

Should we only do this when `DEBUG`?

> Tools/WebKitBot/WebKitBot.mjs:368
> +        console.log("4. Fetching");

Should we only do this when `DEBUG`?

> Tools/WebKitBot/WebKitBot.mjs:374
> +        console.log("Reverting ", revisions, reason);

Should we only do this when `DEBUG`?

> Tools/WebKitBot/WebKitBot.mjs:387
> +        console.log("5. Creating revert patch ", revisions, reason);

Should we only do this when `DEBUG`?

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

Style: indentation

> Tools/WebKitBot/WebKitBot.mjs:417
> +        console.log(stdout);
> +        console.log(stderr);

Should we only do this when `DEBUG`?

> Tools/WebKitBot/WebKitBot.mjs:420
> +            if (bugId != null)

NIT: `!==`

> Tools/WebKitBot/WebKitBot.mjs:425
> +            if (bugId != null)

NIT: `!==`

> Tools/WebKitBot/WebKitBot.mjs:433
> +        console.log(task);

Should we only do this when `DEBUG`?

> Tools/WebKitBot/WebKitBot.mjs:439
> +        case "pull": {

NIT: this doesn't need `{` or `}`

> Tools/WebKitBot/WebKitBot.mjs:455
> +        for (;;) {

Is there a difference/preference over `while (true)`?

> Tools/WebKitBot/index.mjs:30
> +import dotenv from "dotenv"
> +import storage from "node-persist"
> +import WKR from "./WKR.mjs"
> +import WebKitBot from "./WebKitBot.mjs"
> +import SlackWebAPI from "@slack/web-api";

Style: these should all end in `;`

[Yusuke Suzuki]

Comment on attachment 402459 [details]
Patch

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

Thanks for your reviews!!!!
Cool, I'll put some tests in WebKitBot dir. But I'm not sure how to connect these things to test-webkit-scripts. Maybe, I could just put the test usable from `npm test` in this directory.

>> Tools/WebKitBot/AsyncTaskQueue.mjs:30
>> +        this.limit = limit;
> 
> NIT: i think these could both be "private" (prefixed with `_`)

Fixed.

>> Tools/WebKitBot/AsyncTaskQueue.mjs:57
>> +    length()
> 
> NIT: perhaps make this a `get length()`?

Fixed.

>> Tools/WebKitBot/ReadMe.md:5
>> +WKR is fetching git.webkit.org RSS feed periodically, extracting data from that, and posting them to #changes channel to replace IRC's WKR bot purpose.
> 
> Nit: I would remove "to replace..." since it won't be relevant what IRC bot used to do after this patch lands.

Changed, nice.

>> Tools/WebKitBot/WKR.mjs:31
>> +import axios from "axios"
> 
> Style: these should all end in `;`

Fixed.

>> Tools/WebKitBot/WKR.mjs:42
>> +    await new Promise(function (resolve) {
> 
> Style: no space between `function` and `(` if it's anonymous

Fixed.

>> Tools/WebKitBot/WKR.mjs:56
>> +        this.emails = new Map();
> 
> NIT: you can drop the `()` if there are no arguments to a constructor

Fixed.

>> Tools/WebKitBot/WKR.mjs:58
>> +        for (let [fullName, entry] of Object.entries(data)) {
> 
> NIT: rather than iterate `data` twice, perhaps make `this.entries = [];` and then `this.entries.push(entry)` in the loop

Fixed.

>> Tools/WebKitBot/WKR.mjs:86
>> +                continue;
> 
> Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

Fixed.

>> Tools/WebKitBot/WKR.mjs:107
>> +        [this.bugzilla, change] = Commit.findAndRemove(change, /https?:\/\/(?:bugs\.webkit\.org\/show_bug\.cgi\?id=|webkit\.org\/b\/)(\d+)/im);
> 
> this is nice 😃

:D

>> Tools/WebKitBot/WKR.mjs:133
>> +        if (!this.revision)
> 
> NIT: I'd add a newline before this for readability

Nice. Fixed.

>> Tools/WebKitBot/WKR.mjs:134
>> +            throw new Error(`Canont find revision`);
> 
> Nit: Cannot

Ooops, lol, fixed.

>> Tools/WebKitBot/WKR.mjs:145
>> +            results.push(`https://webkit.org/b/${this.bugzilla}`);
> 
> Do you want to pull out radar links too? 😃
> ```
>     [this.radar, change] = Commit.findAndRemove(change, /<rdar:\/\/(?:problem\/)?(\d+)>/im);
> ```

Sounds very nice improvement!!!

>> Tools/WebKitBot/WKR.mjs:155
>> +        this.revision = revision;
> 
> NIT: i think these could all be "private" (prefixed with `_`)

Right, changed.

>> Tools/WebKitBot/WKR.mjs:161
>> +            "text": commit.message()
> 
> Style: no need for quotes around `text`
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WKR.mjs:164
>> +        if (!DEBUG)
> 
> Should we `console.log` the data when `DEBUG` (i.e. in an `else`)?

Yeah, right. Changed.

>> Tools/WebKitBot/WKR.mjs:171
>> +        console.log(`${Date.now()}: poll data`);
> 
> Should we only do this when `DEBUG`?

Chagned.

>> Tools/WebKitBot/WKR.mjs:194
>> +        console.log(`Endpoint: ${process.env.slackURL}`);
> 
> Should we only do this when `DEBUG`?

Changed.

>> Tools/WebKitBot/WKR.mjs:201
>> +        for (;;) {
> 
> Is there a difference/preference over `while (true)`?

This is the same. Either is fine. I've replaced it with `while (true)`.

>> Tools/WebKitBot/WebKitBot.mjs:30
>> +import AsyncTaskQueue from "./AsyncTaskQueue.mjs"
> 
> Style: these should all end in `;`

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:49
>> +        return null;
> 
> Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:52
>> +        return match[1];
> 
> Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:65
>> +            continue;
> 
> Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:68
>> +            return null;
> 
> Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:88
>> +        for (let revision of extracted)
> 
> Nit: Node.js v6 and later supports destructuring.
> 
> revisions.push(...extracted)
> 
> And earlier:
> 
> revisions.push.apply(revisions, extracted)

SOunds like a nice idea. Changed.

>> Tools/WebKitBot/WebKitBot.mjs:96
>> +            if (reason.charAt(reason.length - 1) === firstCharacterOfReason)
> 
> Please check that this implementation supports smart quotes, as this has been a point of frustration in the past with webkitbot.

Yeah, as you found, we are preprocessing the text content which replaces smart quotes with non-smart quotes a priori :)

>> Tools/WebKitBot/WebKitBot.mjs:101
>> +    return {
> 
> Nit: this could be on one line.

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:118
>> +        this.auth = auth;
> 
> NIT: i think these could all be "private" (prefixed with `_`)

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:120
>> +        this.commands = new Map();
> 
> NIT: you can drop the `()` if there are no arguments to a constructor

OK, fixed.

>> Tools/WebKitBot/WebKitBot.mjs:135
>> +            description: "Responds with pong.",
> 
> NIT: you could add a "to check if WebKitBot is alive/working" to further explain this :)

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:159
>> +            case "message": {
> 
> If you only have one `case`, why not just make this into an early-return `if (event.type !== "message") return;`?

I was thinking about handling more types, but it is super likely that only "message" type will be handled in WebKitBot! So yeah, early-return sounds preferable. Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:162
>> +                    return;
> 
> Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

Nice, fixed.

>> Tools/WebKitBot/WebKitBot.mjs:181
>> +            await this.taskQueue.postOrFailWhenExceedingLimit({
> 
> Making this `async` doesn't actually do anything IIUC because `setInterval` won't wait on the result of an `async` function before scheduling the next call.  If you want the next call to be scheduled after the previous call finishes, I'd do
> ```
>     setTimeout(async function pull() {
>         await this.taskQueue.postOrFailWhenExceedingLimit({
>             command: "pull",
>         });
> 
>         setTimeout(pull, defaultPullPeriod);
>     }, defaultPullPeriod);
> ```
> otherwise, I'd drop the `async` and `await` as they don't really do anything :/

The purpose of this periodic task is that periodically posting a job of "pull" to the queue so that we ensure that working copy of WebKit is up-to-date.
And the job could fail, because of size limit of AsyncTaskQueue. So regardless of whether the previous task succeeded or not, we would like post a task periodically.
And we also do not need to wait the finish of the previous task. That's why this function is just posting a task. I think that no `async` is OK. I was using `async` just to write some code after `await this.taskQueue.postOrFailWhenExceedingLimit`, but I didn't add anything.

>> Tools/WebKitBot/WebKitBot.mjs:193
>> +            return null;
> 
> Style: I'd add a newline after control flow that precedes a larger block of code so that it's clear there's a branch in logic

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:197
>> +        // 1. Convert smart quotes to normal ASCII quotes because webkit-patch cannot accept non-ASCII text and slack may convert normal quotes to smart quotes.
> 
> EDIT: Ah, I see, this is handled at a higher level.

Yeah, we are removing smart quotes in preprocessing phase. The main reason of this is webkit-patch create-revert does not accept non-ASCII "revert reason". So we do not want to produce such a "revert reason".
https://bugs.webkit.org/show_bug.cgi?id=212425

>> Tools/WebKitBot/WebKitBot.mjs:209
>> +			await this.web.chat.postMessage({
> 
> Style: indentation

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:211
>> +				text: `<@${event.user}> webkit-patch only accepts an ASCII string for reason: \`${escapeForSlackText(reason)}\``
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:216
>> +        console.log(revisions, reason);
> 
> Should we only do this when `DEBUG`?

Yeah, it sounds OK. Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:230
>> +                    text: `<@${event.user}> Created a revert patch https://webkit.org/b/${escapeForSlackText(bugId)}`
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:236
>> +                    text: `<@${event.user}> Failed to create revert patch.`
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:243
>> +            text: `<@${event.user}> Failed to parse revision and reason`
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:252
>> +			await this.web.chat.postMessage({
> 
> Style: indentation

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:254
>> +				text: `<@${event.user}> webkit-patch only accepts an ASCII string for reason: \`${escapeForSlackText(reason)}\``
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:262
>> +                text: `<@${event.user}> No revision is found: reason = \`${escapeForSlackText(reason)}\``
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:269
>> +            text: `<@${event.user}> revisions = \`${escapeForSlackText(revisions.join(","))}\`, reason = \`${escapeForSlackText(reason)}\``
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:277
>> +            text: `<@${event.user}> Preparing pulling the latest WebKit checkout.`
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:284
>> +            text: `<@${event.user}> Pulled the latest checkout.`
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:292
>> +            text: `<@${event.user}> pong`
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:304
>> +                    text: `<@${event.user}> "${escapeForSlackText(commandName)}": ${escapeForSlackText(operation.description)}\nUsage: ${escapeForSlackText(operation.usage)}`
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:309
>> +                    text: `<@${event.user}> Unknown command "${escapeForSlackText(commandName)}"`
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:318
>> +                text: `<@${event.user}> Available commands: ${escapeForSlackText(commandNames.join(", "))}\nType "help COMMAND" for help on my individual commands.`
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:327
>> +            text: `<@${event.user}> ${escapeForSlackText(this.taskQueue.length())} requests in queue.`
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:333
>> +        console.log("Unknown command: ", command);
> 
> Should we only do this when `DEBUG`?

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:336
>> +            text: `<@${event.user}> Unknown command "${escapeForSlackText(command)}"`
> 
> Style: add trailing comma

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:348
>> +            task.on('close', (code) => {
> 
> Style: double quotes

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:359
>> +        console.log("1. Resetting");
> 
> Should we only do this when `DEBUG`?

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:362
>> +        console.log("2. Cleaning");
> 
> Should we only do this when `DEBUG`?

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:365
>> +        console.log("3. Pulling");
> 
> Should we only do this when `DEBUG`?

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:368
>> +        console.log("4. Fetching");
> 
> Should we only do this when `DEBUG`?

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:374
>> +        console.log("Reverting ", revisions, reason);
> 
> Should we only do this when `DEBUG`?

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:387
>> +        console.log("5. Creating revert patch ", revisions, reason);
> 
> Should we only do this when `DEBUG`?

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:391
>> +                    "create-revert",
> 
> Style: indentation

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:417
>> +        console.log(stderr);
> 
> Should we only do this when `DEBUG`?

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:420
>> +            if (bugId != null)
> 
> NIT: `!==`

I did this to handle null and undefined, but possibly null is enough. Check and fix.

>> Tools/WebKitBot/WebKitBot.mjs:425
>> +            if (bugId != null)
> 
> NIT: `!==`

I did this to handle null and undefined, but possibly null is enough. Check and fix.

>> Tools/WebKitBot/WebKitBot.mjs:433
>> +        console.log(task);
> 
> Should we only do this when `DEBUG`?

Fixed.

>> Tools/WebKitBot/WebKitBot.mjs:439
>> +        case "pull": {
> 
> NIT: this doesn't need `{` or `}`

Right, fixed.

>> Tools/WebKitBot/WebKitBot.mjs:455
>> +        for (;;) {
> 
> Is there a difference/preference over `while (true)`?

That's the same. Changed.

>> Tools/WebKitBot/index.mjs:30
>> +import SlackWebAPI from "@slack/web-api";
> 
> Style: these should all end in `;`

Fixed.

[Yusuke Suzuki]

Created attachment 402633 [details]
Patch

WIP: resolved comments except testing. Introduced lint

[Yusuke Suzuki]

Created attachment 402710 [details]
Patch

[Yusuke Suzuki]

Created attachment 402711 [details]
Patch

[Devin Rousso]

Comment on attachment 402711 [details]
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/command"

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?

[Yusuke Suzuki]

Comment on attachment 402711 [details]
Patch

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

Thanks!

>> Tools/ChangeLog:50
>> +        * WebKitBot/tests/resources/NoRadarAndBugzilla.json: Added.
> 
> NIT: not sure why, but these are showing as binary files in this diff 🤔

I did it because the content of this JSON is not so important. I attached .gitattributes to make them binary.

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

Fixed.

>> 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.

Nice, yes, this is redundant and unnecessary. Fixed.
And I found that there is ESLint rule to detect it. I've enabled it too.
https://github.com/eslint/eslint/blob/master/docs/rules/no-return-await.md

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

Fixed.

>> Tools/WebKitBot/src/AsyncTaskQueue.mjs:68
>> +                reject
> 
> Style: missing trailing comma

Fixed. And changed eslintrc rule from only-multiline to always-multiline.

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

Right. Changed.

>> Tools/WebKitBot/src/AsyncTaskQueue.mjs:77
>> +            this.resolve = resolve;
> 
> this should probably be "private", and could probably use a better name like `_conditionPromiseResolve`

Right, changed.

>> Tools/WebKitBot/src/Commit.mjs:47
>> +            change = replaceAll(entry.fullName, nameWithNicks, change);
> 
> NIT: do we really need a package for this 😓

I've checked but the latest node.js is not including V8 which implements replaceAll (note that JSC implemented it and shipped :)).

>> Tools/WebKitBot/src/Commit.mjs:100
>> +            this._bugzilla = null;
> 
> NIT: you could use a ternary

Fixed.

>> Tools/WebKitBot/src/Commit.mjs:104
>> +            this._radar = null;
> 
> Ditto (:97)

Fixed.

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

Fixed.

>> Tools/WebKitBot/src/WebKitBot.mjs:93
>> +        if (firstCharacterOfReason === "'" || firstCharacterOfReason === "\"") {
> 
> Should we also include "`"?

We can include it. Changed.

>> 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"

Fixed.

>> 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)

Fixed.

>> Tools/WebKitBot/src/WebKitBot.mjs:199
>> +    async revertCommand(event, command, args)
> 
> NIT: I feel like most of these methods should be "private"

For now, I would like to keep them public since this is interface to WebKitBot, and it would be possible that we will invoke this.

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

Fixed.

>> 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 🤩

Yes, we can do that :) This is good part of Slack compared to IRC!

>> Tools/WebKitBot/src/WebKitBot.mjs:236
>> +        await this._web.chat.postMessage({
> 
> Style: I'd add a newline after the `}` since there is a `return`

Added.

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

I've set `"indent": ["error", 4]` in eslint. And fixed.

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

Fixed.

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

Fixed.

>> Tools/WebKitBot/tests/Commit.test.mjs:42
>> +    expect(commit.revert).toBe(null);
> 
> Can we add tests for these too?

Currently, it is not supported. We will add tests too once it is supported.

[Yusuke Suzuki]

Committed r264210: <https://trac.webkit.org/changeset/264210>

[Radar WebKit Bug Importer]

<rdar://problem/65317981>

[Yusuke Suzuki]

Committed r264276: <https://trac.webkit.org/changeset/264276>