ronantakizawa/codereview-bench huggingface dataset:
Repository: eslint/eslint
File: docs/_examples/integration-tutorial-code/sample-data/test-file.js (JavaScript)

Code before:
/** 
 * @fileoverview Example data to lint using ESLint. This file contains a variety of errors.
 * @author Ben Perlmutter
 */
v// 'var' should be replaced with 'const' or 'let' (no-var from eslint:recommended)
var x = 10;

// Unused variable 'y' (no-unused-vars from custom rules)
const y = 20;

function add(a, b) {
  // Unexpected console statement (no-console from custom rules)
  console.log('Adding two numbers');
  return a + b;
}

// 'result' is assigned a value but never used (no-unused-vars from custom rules)
const result = add(x, 5);

// Expected indentation of 2 spaces but found 4 (indent from eslint:recommended)
    if (x > 5) {
        // Unexpected console statement (no-console from custom rules)
        console.log('x is greater than 5');
    } else {
        // Unexpected console statement (no-console from custom rules)
        console.log('x is not greater than 5');
    }

// 'subtract' is defined but never used (no-unused-vars from custom rules)
function subtract(a, b) {
  return a - b;
}


Review comment (suggestion):
Same as above, `indent` is not in `eslint:recommended` and it isn't enabled in the hardcoded configuration inside `example-eslint-integration`.

Code after:
/** 
 * @fileoverview Example data to lint using ESLint. This file contains a variety of errors.
 * @author Ben Perlmutter
 */

// Unused variable 'y' (no-unused-vars from configured rules)
const y = 20;

function add(a, b) {
  // Unexpected console statement (no-console from configured rules)
  console.log('Adding two numbers');
  return a + b;
}

// 'result' is assigned a value but never used (no-unused-vars from configured rules)
const result = add(x, 5);

if (x > 5) {
    // Unexpected console statement (no-console from configured rules)
    console.log('x is greater than 5');
} else {
    // Unexpected console statement (no-console from configured rules)
    console.log('x is not greater than 5');
}

// 'subtract' is defined but never used (no-unused-vars from configured rules)
function subtract(a, b) {
  return a - b;
}
Repository: eslint/eslint
File: messages/invalid-rule-options.js (JavaScript)

Code before:
"use strict";

const { stringifyValueForError } = require("./shared");

module.exports = function({ ruleId, value }) {
    return `
Configuration for rule "${ruleId}" is invalid. Expected severity of "off", 0, "warn", 1, "error", or 2.

You passed '${stringifyValueForError(value, 4)}'.

If you're attempting to configure rule options, perhaps you meant:

    ["error", ${stringifyValueForError(value, 8)}]

See https://eslint.org/docs/latest/use/configure/rules#using-configuration-files for configuring rules.
`.trimStart();
};


Review comment (suggestion):
I think in this case we can reasonably assume that the problem is primarily a missing severity.

```suggestion
Configuration for rule "${ruleId}" is invalid. Each rule must have a severity ("off", 0, "warn", 1, "error", or 2) and may be followed by additional options for the rule.

You passed '${stringifyValueForError(value, 4)}', which doesn't contain a valid severity.

If you're attempting to configure rule options, perhaps you meant:

    "${ruleId}": ["error", ${stringifyValueForError(value, 8)}]

See https://eslint.org/docs/latest/use/configure/rules#using-configuration-files for configuring rules.
```

Code after:
"use strict";

const { stringifyValueForError } = require("./shared");

module.exports = function({ ruleId, value }) {
    return `
Configuration for rule "${ruleId}" is invalid. Each rule must have a severity ("off", 0, "warn", 1, "error", or 2) and may be followed by additional options for the rule.

You passed '${stringifyValueForError(value, 4)}', which doesn't contain a valid severity.

If you're attempting to configure rule options, perhaps you meant:

    "${ruleId}": ["error", ${stringifyValueForError(value, 8)}]

See https://eslint.org/docs/latest/use/configure/rules#using-configuration-files for configuring rules.
`.trimStart();
};
Repository: eslint/eslint
File: messages/shared.js (JavaScript)

Code before:
/**
 * @fileoverview Shared utilities for error messages.
 * @author Josh Goldberg
 */

"use strict";

/**
 * Converts a value to a string that may be printed in errors.
 * @param {any} value The invalid value.
 * @param {number} indentation How many spaces to indent
 * @returns {string} The value, stringified.
 */
function stringifyValueForError(value, indentation) {
    return JSON.stringify(value, null, 4).replace(/\n/gu, `\n${" ".repeat(indentation)}`);
}

module.exports = { stringifyValueForError };


Review comment (bug):
This throws on `value = undefined`, and then error reporting [falls back](https://github.com/eslint/eslint/blob/f5574dc739fcc74a7841217ba1f31cce02bee1ff/bin/eslint.js#L76-L83) to logging `error.stack`. For example:

```js
{
    rules: {
        "no-undef": []
    }
}
```

```
Oops! Something went wrong! :(

ESLint: 8.39.0

Error: Key "rules": Key "no-undef": Expected severity of "off", 0, "warn", 1, "error", or 2.
at assertIsRuleSeverity ...
```

Perhaps we could improve this?

Code after:
/**
 * @fileoverview Shared utilities for error messages.
 * @author Josh Goldberg
 */

"use strict";

/**
 * Converts a value to a string that may be printed in errors.
 * @param {any} value The invalid value.
 * @param {number} indentation How many spaces to indent
 * @returns {string} The value, stringified.
 */
function stringifyValueForError(value, indentation) {
    return value ? JSON.stringify(value, null, 4).replace(/\n/gu, `\n${" ".repeat(indentation)}`) : `${value}`;
}

module.exports = { stringifyValueForError };
Repository: eslint/eslint
File: lib/shared/string-utils.js (JavaScript)

Code before:
 */
function upperCaseFirst(string) {
    if (string.length <= 1) {
        return string.toUpperCase();
    }
    return string[0].toUpperCase() + string.slice(1);
}

/**
 * Counts graphemes in a given string.
 * @param {string} value A string to count graphemes.
 * @returns {number} The number of graphemes in `value`.
 */
function getGraphemeCount(value) {
    if (ASCII_REGEX.test(value)) {
        return value.length;
    }

    if (!iterateGraphemes) {
        if (typeof Intl !== "undefined" && typeof Intl.Segmenter !== "undefined") {
            iterateGraphemes = Intl.Segmenter.prototype.segment.bind(new Intl.Segmenter());
        } else {
            const Graphemer = require("graphemer").default;

            iterateGraphemes = Graphemer.prototype.iterateGraphemes.bind(new Graphemer());
        }
    }

    let count = 0;

    // eslint-disable-next-line no-unused-vars -- syntax required
    for (const _ of iterateGraphemes(value)) {
        count++;
    }

    return count;
}

module.exports = {
    upperCaseFirst,
    getGraphemeCount
};


Review comment (suggestion):
Do these two methods always give the same result? If we can't guarantee that, it would be better to switch to just `graphemer` now and then just `Intl.Segmenter` in the next major version (when we drop Node < 16), because we wouldn't want linting to produce different results in different environments.

Code after:
//------------------------------------------------------------------------------

/**
 * Converts the first letter of a string to uppercase.
 * @param {string} string The string to operate on
 * @returns {string} The converted string
 */
function upperCaseFirst(string) {
    if (string.length <= 1) {
        return string.toUpperCase();
    }
    return string[0].toUpperCase() + string.slice(1);
}

/**
 * Counts graphemes in a given string.
 * @param {string} value A string to count graphemes.
 * @returns {number} The number of graphemes in `value`.
 */
function getGraphemeCount(value) {
    if (ASCII_REGEX.test(value)) {
        return value.length;
    }

    if (!splitter) {
        splitter = new Graphemer();
    }

    return splitter.countGraphemes(value);
}

module.exports = {
    upperCaseFirst,
    getGraphemeCount
};
Repository: eslint/eslint
File: tests/lib/eslint/flat-eslint.js (JavaScript)

Code before:
            process.env.ESLINT_USE_FLAT_CONFIG = false;
        });

        afterEach(() => {
            delete process.env.ESLINT_USE_FLAT_CONFIG;
        });

        it("is `false`", async () => {
            assert.strictEqual(await shouldUseFlatConfig(true), false);
        });
    });

    describe("when the env variable `ESLINT_USE_FLAT_CONFIG` is unset", () => {
        describe("when there is a flat config file present", () => {
            const originalDir = process.cwd();

            beforeEach(() => {
                process.chdir(__dirname);
            });

            afterEach(() => {
                process.chdir(originalDir);
            });

            it("is `true`", async () => {
                assert.strictEqual(await shouldUseFlatConfig(true), true);
            });
        });

        describe("when there is no flat config file present", () => {
            const originalDir = process.cwd();

            beforeEach(() => {
                process.chdir(os.tmpdir());
            });

            afterEach(() => {
                process.chdir(originalDir);
            });

            it("is `false`", async () => {
                assert.strictEqual(await shouldUseFlatConfig(true), false);
            });
        });
    });
});


Review comment (suggestion):
```suggestion
                assert.strictEqual(await shouldUseFlatConfig(), true);
```

Code after:
            });

            afterEach(() => {
                process.chdir(originalDir);
            });

            it(`is \`${expectedValueWithConfig}\``, async () => {
                assert.strictEqual(await shouldUseFlatConfig(), expectedValueWithConfig);
            });
        });

        describe("when there is no flat config file present", () => {
            const originalDir = process.cwd();

            beforeEach(() => {
                process.chdir(os.tmpdir());
            });

            afterEach(() => {
                process.chdir(originalDir);
            });

            it(`is \`${expectedValueWithoutConfig}\``, async () => {
                assert.strictEqual(await shouldUseFlatConfig(), expectedValueWithoutConfig);
            });
        });
    }

    describe("when the env variable `ESLINT_USE_FLAT_CONFIG` is `'true'`", () => {
        beforeEach(() => {
            process.env.ESLINT_USE_FLAT_CONFIG = true;
        });

        afterEach(() => {
            delete process.env.ESLINT_USE_FLAT_CONFIG;
        });

        testShouldUseFlatConfig(true, true);
    });

    describe("when the env variable `ESLINT_USE_FLAT_CONFIG` is `'false'`", () => {
        beforeEach(() => {
            process.env.ESLINT_USE_FLAT_CONFIG = false;
        });

        afterEach(() => {
            delete process.env.ESLINT_USE_FLAT_CONFIG;
        });

        testShouldUseFlatConfig(false, false);
    });
Repository: eslint/eslint
File: tests/lib/eslint/flat-eslint.js (JavaScript)

Code before:
            beforeEach(() => {
                process.chdir(__dirname);
            });

            afterEach(() => {
                process.chdir(originalDir);
            });

            it("is `true`", async () => {
                assert.strictEqual(await shouldUseFlatConfig(true), true);
            });
        });

        describe("when there is no flat config file present", () => {
            const originalDir = process.cwd();

            beforeEach(() => {
                process.chdir(os.tmpdir());
            });

            afterEach(() => {
                process.chdir(originalDir);
            });

            it("is `false`", async () => {
                assert.strictEqual(await shouldUseFlatConfig(true), false);
            });
        });
    });
});


Review comment (suggestion):
```suggestion
                assert.strictEqual(await shouldUseFlatConfig(), false);
```

Code after:
            });

            afterEach(() => {
                process.chdir(originalDir);
            });

            it(`is \`${expectedValueWithoutConfig}\``, async () => {
                assert.strictEqual(await shouldUseFlatConfig(), expectedValueWithoutConfig);
            });
        });
    }

    describe("when the env variable `ESLINT_USE_FLAT_CONFIG` is `'true'`", () => {
        beforeEach(() => {
            process.env.ESLINT_USE_FLAT_CONFIG = true;
        });

        afterEach(() => {
            delete process.env.ESLINT_USE_FLAT_CONFIG;
        });

        testShouldUseFlatConfig(true, true);
    });

    describe("when the env variable `ESLINT_USE_FLAT_CONFIG` is `'false'`", () => {
        beforeEach(() => {
            process.env.ESLINT_USE_FLAT_CONFIG = false;
        });

        afterEach(() => {
            delete process.env.ESLINT_USE_FLAT_CONFIG;
        });

        testShouldUseFlatConfig(false, false);
    });

    describe("when the env variable `ESLINT_USE_FLAT_CONFIG` is unset", () => {
        testShouldUseFlatConfig(true, false);
    });
});
Repository: eslint/eslint
File: lib/linter/apply-disable-directives.js (JavaScript)

Code before:
        directives: blockDirectives,
        disableFixes,
        reportUnusedDisableDirectives
    });
    const lineDirectivesResult = applyDirectives({
        problems: blockDirectivesResult.problems,
        directives: lineDirectives,
        disableFixes,
        reportUnusedDisableDirectives
    });

    if (reportUnusedDisableDirectives !== "off") {
        const rulesToIgnore = configuredRules && ruleFilter
            ? new Set(Object.keys(configuredRules).filter(ruleId => {
                const severity = ConfigOps.getRuleSeverity(configuredRules[ruleId]);

                return !ruleFilter({ severity, ruleId });
            }))
            : new Set();

        const unusedDirectives = blockDirectivesResult.unusedDirectives
            .concat(lineDirectivesResult.unusedDirectives)
            .filter(unusedDirective => {
                const ruleId = unusedDirective.ruleId;

                if (ruleId === null && rulesToIgnore.size > 0 || rulesToIgnore.has(ruleId)) {
                    return false;
                }

                return true;
            });

        lineDirectivesResult.problems = lineDirectivesResult.problems
            .concat(unusedDirectives)
            .sort(compareLocations);
    }

    return lineDirectivesResult.problems;
};


Review comment (refactor):
It seems like we could simplify this by adding a `null` to `rulesToIgnore` and then just checking `rulesToIgnore.has(ruleId)`?

Also, a comment here explaining why we are filtering out rules without a `ruleId` would be helpful.



Code after:
        ? new Set(Object.keys(configuredRules).filter(ruleId => {
            const severity = ConfigOps.getRuleSeverity(configuredRules[ruleId]);

            // Ignore for disabled rules.
            if (severity === 0) {
                return false;
            }

            return !ruleFilter({ severity, ruleId });
        }))
        : new Set();

    // If no ruleId is supplied that means this directive is applied to all rules, so we can't determine if it's unused if any rules are filtered out.
    if (rulesToIgnore.size > 0) {
        rulesToIgnore.add(null);
    }

    const blockDirectivesResult = applyDirectives({
        problems,
        directives: blockDirectives,
        disableFixes,
        reportUnusedDisableDirectives,
        rulesToIgnore
    });
    const lineDirectivesResult = applyDirectives({
        problems: blockDirectivesResult.problems,
        directives: lineDirectives,
        disableFixes,
        reportUnusedDisableDirectives,
        rulesToIgnore
    });

    return reportUnusedDisableDirectives !== "off"
        ? lineDirectivesResult.problems
            .concat(blockDirectivesResult.unusedDirectives)
            .concat(lineDirectivesResult.unusedDirectives)
            .sort(compareLocations)
        : lineDirectivesResult.problems;
};
Repository: eslint/eslint
File: lib/rules/no-case-declarations.js (JavaScript)

Code before:
    },

    create(context) {

        /**
         * Checks whether or not a node is a lexical declaration.
         * @param {ASTNode} node A direct child statement of a switch case.
         * @returns {boolean} Whether or not the node is a lexical declaration.
         */
        function isLexicalDeclaration(node) {
            switch (node.type) {
                case "FunctionDeclaration":
                case "ClassDeclaration":
                    return true;
                case "VariableDeclaration":
                    return node.kind !== "var";
                default:
                    return false;
            }
        }

        return {
            SwitchCase(node) {
                if (
                    node.consequent.length === 0 || // Empty clause
                    node.consequent[0].type === "BlockStatement" && node.consequent.length === 1 // Clause has {} around child expressions
                ) {
                    return;
                }

                for (let i = 0; i < node.consequent.length; i++) {
                    const statement = node.consequent[i];

                    if (isLexicalDeclaration(statement)) {
                        context.report({
                            node: statement,
                            messageId: "unexpected"
                        });
                    }
                }
            }
        };

    }
};


Review comment (suggestion):
Better to check "the only element" part earlier
```suggestion
                    node.consequent.length === 1 && node.consequent[0].type === "BlockStatement"  // Clause has {} around child expressions
```

Code after:
    },

    create(context) {

        /**
         * Checks whether or not a node is a lexical declaration.
         * @param {ASTNode} node A direct child statement of a switch case.
         * @returns {boolean} Whether or not the node is a lexical declaration.
         */
        function isLexicalDeclaration(node) {
            switch (node.type) {
                case "FunctionDeclaration":
                case "ClassDeclaration":
                    return true;
                case "VariableDeclaration":
                    return node.kind !== "var";
                default:
                    return false;
            }
        }

        return {
            SwitchCase(node) {
                for (let i = 0; i < node.consequent.length; i++) {
                    const statement = node.consequent[i];

                    if (isLexicalDeclaration(statement)) {
                        context.report({
                            node: statement,
                            messageId: "unexpected"
                        });
                    }
                }
            }
        };

    }
};
Repository: eslint/eslint
File: messages/eslintrc-plugins.js (JavaScript)

Code before:
"use strict";

module.exports = function({ plugins }) {

    return `
A config object has a "plugins" key defined as an array of strings.

Flat config requires "plugins" to be an object in this form:

    {
        plugins: {
            ${plugins[0] || "namespace"}: pluginObject
        }
    }

Please see the following page for information on how to convert your config object into the correct format:
https://eslint.org/docs/latest/use/configure/migration-guide#importing-plugins-and-custom-parsers

If you're using a shareable config that you cannot rewrite in flat config format, then use the compatibility utility:
https://eslint.org/docs/latest/use/configure/migration-guide#using-eslintrc-configs-in-flat-config
`;
};


Review comment (bug):
Flat config schema throws this error on any array (it doesn't check whether it's an array of _strings_), so if the user specifies plugins like this:

```js
module.exports = [{
    plugins: [somePluginObject]
}];
``` 

The printed message will look like this:

```
Oops! Something went wrong! :(

ESLint: 8.45.0


A config object has a "plugins" key defined as an array of strings.

Flat config requires "plugins" to be an object in this form:

    {
        plugins: {
            [object Object]: pluginObject
        }
    }

Please see the following page for information on how to convert your config object into the correct format:
https://eslint.org/docs/latest/use/configure/migration-guide#importing-plugins-and-custom-parsers

If you're using a shareable config that you cannot rewrite in flat config format, then use the compatibility utility:
https://eslint.org/docs/latest/use/configure/migration-guide#using-eslintrc-configs-in-flat-config
```

Perhaps we should have two templates?

* `plugins` is an array of strings (or empty array?) - possible eslintrc config, use this template
* `plugins` is an array but not array of strings - use another template.

Code after:
"use strict";

module.exports = function({ plugins }) {

    const isArrayOfStrings = typeof plugins[0] === "string";

    return `
A config object has a "plugins" key defined as an array${isArrayOfStrings ? " of strings" : ""}.

Flat config requires "plugins" to be an object in this form:

    {
        plugins: {
            ${isArrayOfStrings && plugins[0] ? plugins[0] : "namespace"}: pluginObject
        }
    }

Please see the following page for information on how to convert your config object into the correct format:
https://eslint.org/docs/latest/use/configure/migration-guide#importing-plugins-and-custom-parsers

If you're using a shareable config that you cannot rewrite in flat config format, then use the compatibility utility:
https://eslint.org/docs/latest/use/configure/migration-guide#using-eslintrc-configs-in-flat-config
`;
};