ronantakizawa/codereview-bench huggingface dataset:
Repository: eslint/eslint
File: lib/rules/no-useless-constructor.js (JavaScript)

Code before:
				hasUsefulAccessibility(node)
			) {
				return;
			}

			/*
			 * Prevent crashing on parsers which do not require class constructor
			 * to have a body, e.g. typescript and flow
			 */
			if (!node.value.body) {
				return;
			}

			const body = node.value.body.body;
			const ctorParams = node.value.params;
			const superClass = node.parent.parent.superClass;

			if (
				superClass
					? isRedundantSuperCall(body, ctorParams)
					: body.length === 0
			) {
				context.report({
					loc: {
						start: node.loc.start,
						end: context.sourceCode.getFirstToken(
							node,
							astUtils.isOpeningParenToken,
						).loc.start,
					},
					messageId: "noUselessConstructor",
					suggest: [
						{
							messageId: "removeConstructor",
							*fix(fixer) {
								const nextToken =
									context.sourceCode.getTokenAfter(node);
								const addSemiColon =
									nextToken.type === "Punctuator" &&
									nextToken.value === "[" &&
									astUtils.needsPrecedingSemicolon(
										context.sourceCode,
										node,
									);

								yield fixer.replaceText(
									node,
									addSemiColon ? ";" : "",
								);
							},
						},

Review comment (refactor):
```suggestion
						end: sourceCode.getFirstToken(
```

Could we also use destructuring (or a similar approach) here to get the `sourceCode` instance at the top of `create(context)`?

`context.sourceCode` is accessed in three places, so extracting it would be small but could reduce repeated lookups.

Code after:
				node.value.type !== "FunctionExpression" ||
				hasDecoratorsOrParameterProperty(node) ||
				hasUsefulAccessibility(node)
			) {
				return;
			}

			/*
			 * Prevent crashing on parsers which do not require class constructor
			 * to have a body, e.g. typescript and flow
			 */
			if (!node.value.body) {
				return;
			}

			const body = node.value.body.body;
			const ctorParams = node.value.params;
			const superClass = node.parent.parent.superClass;
			const parenToken = sourceCode.getFirstToken(
				node,
				astUtils.isOpeningParenToken,
			);
			const loc = {
				start: node.loc.start,
				end: sourceCode.getTokenBefore(parenToken).loc.end,
			};

			if (
				superClass
					? isRedundantSuperCall(body, ctorParams)
					: body.length === 0
			) {
				context.report({
					loc,
					messageId: "noUselessConstructor",
					suggest: [
						{
							messageId: "removeConstructor",
							*fix(fixer) {
								const nextToken =
									sourceCode.getTokenAfter(node);
								const addSemiColon =
									nextToken.type === "Punctuator" &&
									nextToken.value === "[" &&
									astUtils.needsPrecedingSemicolon(
										sourceCode,
										node,
									);

								yield fixer.replaceText(
									node,
Repository: eslint/eslint
File: tests/lib/linter/interpolate.js (JavaScript)

Code before:
		assert.deepStrictEqual(matches, [[text, " placeholder "]]);
	});
});

describe("interpolate()", () => {
	it("passes through text without {{ }}", () => {
		const message = "This is a very important message!";

		assert.strictEqual(interpolate(message, {}), message);
	});
	it("passes through text with {{ }} that don’t match a key", () => {
		const message = "This is a very important {{ message }}!";

		assert.strictEqual(interpolate(message, {}), message);
	});
	it("Properly interpolates keys in {{ }}", () => {
		assert.strictEqual(
			interpolate("This is a very important {{ message }}!", {
				message: "test",
			}),
			"This is a very important test!",
		);
	});

	const values = [
		// Expected
		[1, "1"],
		[100n, "100"],
		[true, "true"],
		[false, "false"],
		[void 0, "undefined"],
		[null, "null"],

		// Unexpected
		[{ prop: "value" }, "[object Object]"],
		[[1, 2, 3], "1,2,3"],
		[new Set([1, 2, 3]), "[object Set]"],
		[new Map([["prop", "value"]]), "[object Map]"],
	];
	for (const [value, message] of values) {
		it(`stringifies the value ${value}`, () => {
			assert.strictEqual(interpolate("{{ value }}", { value }), message);
		});
	}
});


Review comment (suggestion):
```suggestion
		// Expected, officially supported.
```

Just to note that this behavior should not change.

Code after:
		assert.deepStrictEqual(matches, [[text, " placeholder "]]);
	});
});

describe("interpolate()", () => {
	it("passes through text without {{ }}", () => {
		const message = "This is a very important message!";

		assert.strictEqual(interpolate(message, {}), message);
	});
	it("passes through text with {{ }} that don’t match a key", () => {
		const message = "This is a very important {{ message }}!";

		assert.strictEqual(interpolate(message, {}), message);
	});
	it("Properly interpolates keys in {{ }}", () => {
		assert.strictEqual(
			interpolate("This is a very important {{ message }}!", {
				message: "test",
			}),
			"This is a very important test!",
		);
	});

	const values = [
		// Expected (officially supported)
		[1, "1"],
		[100n, "100"],
		[true, "true"],
		[false, "false"],
		[void 0, "undefined"],
		[null, "null"],

		// Unexpected
		[{ prop: "value" }, "[object Object]"],
		[{ toString: () => "stringified" }, "stringified"],
		[[1, 2, 3], "1,2,3"],
		[new Set([1, 2, 3]), "[object Set]"],
		[new Map([["prop", "value"]]), "[object Map]"],
	];
	for (const [value, message] of values) {
		it(`stringifies the value ${value}`, () => {
			assert.strictEqual(interpolate("{{ value }}", { value }), message);
		});
	}
});
Repository: eslint/eslint
File: tests/lib/linter/interpolate.js (JavaScript)

Code before:
	});
	it("passes through text with {{ }} that don’t match a key", () => {
		const message = "This is a very important {{ message }}!";

		assert.strictEqual(interpolate(message, {}), message);
	});
	it("Properly interpolates keys in {{ }}", () => {
		assert.strictEqual(
			interpolate("This is a very important {{ message }}!", {
				message: "test",
			}),
			"This is a very important test!",
		);
	});

	const values = [
		// Expected
		[1, "1"],
		[100n, "100"],
		[true, "true"],
		[false, "false"],
		[void 0, "undefined"],
		[null, "null"],

		// Unexpected
		[{ prop: "value" }, "[object Object]"],
		[[1, 2, 3], "1,2,3"],
		[new Set([1, 2, 3]), "[object Set]"],
		[new Map([["prop", "value"]]), "[object Map]"],
	];
	for (const [value, message] of values) {
		it(`stringifies the value ${value}`, () => {
			assert.strictEqual(interpolate("{{ value }}", { value }), message);
		});
	}
});


Review comment (suggestion):
```suggestion
		[{ prop: "value" }, "[object Object]"],
		[
			{
				toString() {
					return "Hello";
				},
			},
			"Hello",
		],
```

I'd add this case so that we are aware of it if we ever consider disallowing non-primitive types. 

Code after:
	});
	it("passes through text with {{ }} that don’t match a key", () => {
		const message = "This is a very important {{ message }}!";

		assert.strictEqual(interpolate(message, {}), message);
	});
	it("Properly interpolates keys in {{ }}", () => {
		assert.strictEqual(
			interpolate("This is a very important {{ message }}!", {
				message: "test",
			}),
			"This is a very important test!",
		);
	});

	const values = [
		// Expected (officially supported)
		[1, "1"],
		[100n, "100"],
		[true, "true"],
		[false, "false"],
		[void 0, "undefined"],
		[null, "null"],

		// Unexpected
		[{ prop: "value" }, "[object Object]"],
		[{ toString: () => "stringified" }, "stringified"],
		[[1, 2, 3], "1,2,3"],
		[new Set([1, 2, 3]), "[object Set]"],
		[new Map([["prop", "value"]]), "[object Map]"],
	];
	for (const [value, message] of values) {
		it(`stringifies the value ${value}`, () => {
			assert.strictEqual(interpolate("{{ value }}", { value }), message);
		});
	}
});
Repository: eslint/eslint
File: lib/rules/utils/ast-utils.js (JavaScript)

Code before:
		token.type === "Line" ||
		token.type === "Block" ||
		token.type === "Shebang"
	);
}

/**
 * Checks if the given token is a keyword token or not.
 * @param {Token} token The token to check.
 * @returns {boolean} `true` if the token is a keyword token.
 */
function isKeywordToken(token) {
	return token.type === "Keyword";
}

/**
 * Check to see if its a ES6 export declaration.
 * @param {ASTNode} astNode An AST node.
 * @returns {boolean} whether the given node represents an export declaration.
 * @private
 */
function looksLikeExport(astNode) {
	return (
		astNode.type === "ExportDefaultDeclaration" ||
		astNode.type === "ExportNamedDeclaration"
	);
}

/**
 * Checks for the presence of a JSDoc comment for the given node and returns it.
 * @param {ASTNode} astNode The AST node to get the comment for.
 * @param {SourceCode} sourceCode A SourceCode instance to get comments.
 * @returns {Token|null} The Block comment token containing the JSDoc comment for the given node or null if not found.
 * @private
 */
function findJSDocComment(astNode, sourceCode) {
	const tokenBefore = sourceCode.getTokenBefore(astNode, {
		includeComments: true,
	});

	if (
		tokenBefore &&
		tokenBefore.type === "Block" &&
		tokenBefore.value.charAt(0) === "*" &&
		astNode.loc.start.line - tokenBefore.loc.end.line <= 1
	) {
		return tokenBefore;
	}

	return null;
}

Review comment (refactor):
I think it would be good to keep `ExportAllDeclaration` here so that the behavior matches the description 
because other functions could use this one in the future. Also, since we're already refactoring this, `isExportDeclaration` might be a better name for this function.

Code after:
		token.type === "Line" ||
		token.type === "Block" ||
		token.type === "Shebang"
	);
}

/**
 * Checks if the given token is a keyword token or not.
 * @param {Token} token The token to check.
 * @returns {boolean} `true` if the token is a keyword token.
 */
function isKeywordToken(token) {
	return token.type === "Keyword";
}

/**
 * Checks whether the given node represents an ES6 export declaration.
 * @param {ASTNode} node A node to check.
 * @returns {boolean} `true` if the node is an export declaration.
 * @private
 */
function isExportDeclaration(node) {
	return (
		node.type === "ExportDefaultDeclaration" ||
		node.type === "ExportNamedDeclaration" ||
		node.type === "ExportAllDeclaration"
	);
}

/**
 * Checks for the presence of a JSDoc comment for the given node and returns it.
 * @param {ASTNode} node The node to get the comment for.
 * @param {SourceCode} sourceCode A SourceCode instance to get comments.
 * @returns {Token|null} The Block comment token containing the JSDoc comment for the given node or null if not found.
 * @private
 */
function findJSDocComment(node, sourceCode) {
	const tokenBefore = sourceCode.getTokenBefore(node, {
		includeComments: true,
	});

	if (
		tokenBefore &&
		tokenBefore.type === "Block" &&
		tokenBefore.value.charAt(0) === "*" &&
		node.loc.start.line - tokenBefore.loc.end.line <= 1
	) {
		return tokenBefore;
	}

	return null;
Repository: eslint/eslint
File: lib/rules/no-restricted-imports.js (JavaScript)

Code before:
/**
 * @fileoverview Restrict usage of specified node imports.
 * @author Guy Ellis
 */
"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const astUtils = require("./utils/ast-utils");

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

const ignore = require("ignore");

const arrayOfStringsOrObjects = {
	type: "array",
	items: {
		anyOf: [
			{ type: "string" },
			{
				type: "object",
				properties: {
					name: { type: "string" },
					message: {
						type: "string",
						minLength: 1,
					},
					importNames: {
						type: "array",
						items: {
							type: "string",
						},
					},
					allowImportNames: {
						type: "array",
						items: {
							type: "string",
						},
					},
					allowTypeImports: {
						type: "boolean",
						description:
							"Whether to allow type-only imports for a path.",
					},

Review comment (refactor):
This could throw an error if [Node.js is built without ICU](https://nodejs.org/api/intl.html#detecting-internationalization-support) as Intl is then not available.
Or in general I think this should be a utility as other rules also use lists in their messages.

Code after:
/**
 * @fileoverview Restrict usage of specified node imports.
 * @author Guy Ellis
 */
"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const astUtils = require("./utils/ast-utils");

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------

/**
 * Format import names for error messages.
 * @param {string[]} importNames The import names to format.
 * @returns {string} The formatted import names.
 */
function formatImportNames(importNames) {
	return new Intl.ListFormat("en-US").format(
		importNames.map(name => `'${name}'`),
	);
}

/**
 * Returns "is" or "are" based on the number of import names.
 * @param {string[]} importNames The import names to check.
 * @returns {string} "is" if one import name, otherwise "are".
 */
function isOrAre(importNames) {
	return importNames.length === 1 ? "is" : "are";
}

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

const ignore = require("ignore");

const arrayOfStringsOrObjects = {
	type: "array",
	items: {
		anyOf: [
			{ type: "string" },
			{
Repository: eslint/eslint
File: lib/services/warning-service.js (JavaScript)

Code before:
	 * @returns {void}
	 */
	emitInactiveFlagWarning(flag, message) {
		this.emitWarning(message, `ESLintInactiveFlag_${flag}`);
	}

	/**
	 * Emits a warning when a suboptimal concurrency setting is detected.
	 * Currently, this is only used to warn when the net linting ratio is low.
	 * @param {string} notice A notice about how to improve performance.
	 * @returns {void}
	 */
	emitPoorConcurrencyWarning(notice) {
		this.emitWarning(
			`You may ${notice} to improve performance.`,
			"ESLintPoorConcurrencyWarning",
		);
	}

	/**
	 * Emits a warning when eslint-env configuration comments are found.
	 * @returns {void}
	 */
	emitESLintEnvWarning() {
		this.emitWarning(
			"/* eslint-env */ comments are no longer recognized when linting with flat config and will be reported as errors in v10.0.0. Replace them with /* global */ comments or define globals in your config file. See https://eslint.org/docs/latest/use/configure/migration-guide#eslint-env-configuration-comments for details.",
			"ESLintEnvWarning",
		);
	}
}

module.exports = { WarningService };


Review comment (suggestion):
Suggest to change `v10.0.0` to either `v10.x` or just `v10`, otherwise it sounds as though only the first release of ESLint v10 would show the error.

Code after:
	 * @returns {void}
	 */
	emitInactiveFlagWarning(flag, message) {
		this.emitWarning(message, `ESLintInactiveFlag_${flag}`);
	}

	/**
	 * Emits a warning when a suboptimal concurrency setting is detected.
	 * Currently, this is only used to warn when the net linting ratio is low.
	 * @param {string} notice A notice about how to improve performance.
	 * @returns {void}
	 */
	emitPoorConcurrencyWarning(notice) {
		this.emitWarning(
			`You may ${notice} to improve performance.`,
			"ESLintPoorConcurrencyWarning",
		);
	}

	/**
	 * Emits a warning when eslint-env configuration comments are found.
	 * @param {string} filename The name of the file being linted.
	 * @param {number} line The line number of the comment.
	 * @returns {void}
	 */
	emitESLintEnvWarning(filename, line) {
		this.emitWarning(
			`/* eslint-env */ comments are no longer recognized when linting with flat config and will be reported as errors as of v10.0.0. Replace them with /* global */ comments or define globals in your config file. See https://eslint.org/docs/latest/use/configure/migration-guide#eslint-env-configuration-comments for details. Found in ${filename} at line ${line}.`,
			"ESLintEnvWarning",
		);
	}
}

module.exports = { WarningService };
Repository: eslint/eslint
File: lib/types/index.d.ts (TypeScript)

Code before:
>;

// #region Linter

export class Linter {
	static readonly version: string;

	version: string;

	constructor(options?: { cwd?: string | undefined; configType?: "flat" });

	verify(
		code: SourceCode | string,
		config: Linter.LegacyConfig | Linter.Config | Linter.Config[],
		filename?: string,
	): Linter.LintMessage[];
	verify(
		code: SourceCode | string,
		config: Linter.LegacyConfig | Linter.Config | Linter.Config[],
		options: Linter.LintOptions,
	): Linter.LintMessage[];

	verifyAndFix(
		code: string,
		config: Linter.LegacyConfig | Linter.Config | Linter.Config[],
		filename?: string,
	): Linter.FixReport;
	verifyAndFix(
		code: string,
		config: Linter.LegacyConfig | Linter.Config | Linter.Config[],
		options: Linter.FixOptions,
	): Linter.FixReport;

	getSourceCode(): SourceCode;

	getTimes(): Linter.Stats["times"];

	getFixPassCount(): Linter.Stats["fixPasses"];
}

export namespace Linter {
	/**
	 * The numeric severity level for a rule.
	 *
	 * - `0` means off.
	 * - `1` means warn.
	 * - `2` means error.
	 *
	 * @see [Rule Severities](https://eslint.org/docs/latest/use/configure/rules#rule-severities)
	 */
	type Severity = SeverityLevel;

Review comment (suggestion):
I'm not sure if `verifyAndFix()` was supposed to allow a string as a shortcut for `options.filename`, as it is clearly the case for the `verify()` method. The documentation suggests so, but the logic is a bit inconsistent and this case is not tested.

Code after:
>;

// #region Linter

export class Linter {
	static readonly version: string;

	version: string;

	constructor(options?: { cwd?: string | undefined; configType?: "flat" });

	verify(
		code: SourceCode | string,
		config: Linter.Config | Linter.Config[],
		filename?: string,
	): Linter.LintMessage[];
	verify(
		code: SourceCode | string,
		config: Linter.Config | Linter.Config[],
		options: Linter.LintOptions,
	): Linter.LintMessage[];

	verifyAndFix(
		code: string,
		config: Linter.Config | Linter.Config[],
		filename?: string,
	): Linter.FixReport;
	verifyAndFix(
		code: string,
		config: Linter.Config | Linter.Config[],
		options: Linter.FixOptions,
	): Linter.FixReport;

	getSourceCode(): SourceCode;

	getTimes(): Linter.Stats["times"];

	getFixPassCount(): Linter.Stats["fixPasses"];
}

export namespace Linter {
	/**
	 * The numeric severity level for a rule.
	 *
	 * - `0` means off.
	 * - `1` means warn.
	 * - `2` means error.
	 *
	 * @see [Rule Severities](https://eslint.org/docs/latest/use/configure/rules#rule-severities)
	 */
	type Severity = SeverityLevel;
Repository: eslint/eslint
File: lib/rules/array-callback-return.js (JavaScript)

Code before:

				context.report({
					node,
					loc: astUtils.getFunctionHeadLoc(node, sourceCode),
					messageId: messageAndSuggestions.messageId,
					data: {
						name,
						arrayMethodName: fullMethodName(
							funcInfo.arrayMethodName,
						),
					},
					suggest:
						messageAndSuggestions.suggest.length !== 0
							? messageAndSuggestions.suggest
							: null,
				});
			}
		}

		return {
			// Stacks this function's information.
			onCodePathStart(codePath, node) {
				let methodName = null;

				if (TARGET_NODE_TYPE.test(node.type)) {
					methodName = getArrayMethodName(
						node,
						options.checkArrayFromAsync,
					);
				}

				funcInfo = {
					arrayMethodName: methodName,
					upper: funcInfo,
					codePath,
					hasReturn: false,
					shouldCheck: !!methodName,
					node,
					currentSegments: new Set(),
				};
			},

			// Pops this function's information.
			onCodePathEnd() {
				funcInfo = funcInfo.upper;
			},

			onUnreachableCodePathSegmentStart(segment) {
				funcInfo.currentSegments.add(segment);
			},


Review comment (suggestion):
I've moved the `!node.async` and `!node.generator` checks into `getArrayMethodName`.

Code after:
							funcInfo.arrayMethodName,
						),
					},
					suggest:
						messageAndSuggestions.suggest.length !== 0
							? messageAndSuggestions.suggest
							: null,
				});
			}
		}

		return {
			// Stacks this function's information.
			onCodePathStart(codePath, node) {
				let methodName = null;

				if (TARGET_NODE_TYPE.test(node.type)) {
					methodName = getArrayMethodName(node);
				}

				funcInfo = {
					arrayMethodName: methodName,
					upper: funcInfo,
					codePath,
					hasReturn: false,
					shouldCheck: !!methodName,
					node,
					currentSegments: new Set(),
				};
			},

			// Pops this function's information.
			onCodePathEnd() {
				funcInfo = funcInfo.upper;
			},

			onUnreachableCodePathSegmentStart(segment) {
				funcInfo.currentSegments.add(segment);
			},

			onUnreachableCodePathSegmentEnd(segment) {
				funcInfo.currentSegments.delete(segment);
			},

			onCodePathSegmentStart(segment) {
				funcInfo.currentSegments.add(segment);
			},

			onCodePathSegmentEnd(segment) {
				funcInfo.currentSegments.delete(segment);
			},
Repository: eslint/eslint
File: lib/types/rules.d.ts (TypeScript)

Code before:
		  >;

	/**
	 * Rule to enforce `return` statements in callbacks of array methods.
	 *
	 * @since 2.0.0-alpha-1
	 * @see https://eslint.org/docs/latest/rules/array-callback-return
	 */
	"array-callback-return": Linter.RuleEntry<
		[
			Partial<{
				/**
				 * @default false
				 */
				allowImplicit: boolean;
				/**
				 * @default false
				 */
				checkForEach: boolean;
				/**
				 * @default false
				 */
				allowVoid: boolean;
				/**
				 * @default false
				 * @since 10.x.x
				 */
				checkArrayFromAsync: boolean;
			}>,
		]
	>;

	/**
	 * Rule to enforce line breaks after each array element.
	 *
	 * @since 4.0.0-rc.0
	 * @deprecated since 8.53.0.
	 * Formatting rules are being moved out of ESLint core.
	 * Please, use [`array-element-newline`](https://eslint.style/rules/array-element-newline) in [`@stylistic/eslint-plugin`](https://eslint.style).
	 * @see https://eslint.org/docs/latest/rules/array-element-newline
	 */
	"array-element-newline": Linter.RuleEntry<
		[
			| "always"
			| "never"
			| "consistent"
			| Partial<{
					/**
					 * @default true
					 */
					multiline: boolean;

Review comment (suggestion):
TBD before merging:

```suggestion
				 * @since 10.0.0
```

Code after:
		  >;

	/**
	 * Rule to enforce `return` statements in callbacks of array methods.
	 *
	 * @since 2.0.0-alpha-1
	 * @see https://eslint.org/docs/latest/rules/array-callback-return
	 */
	"array-callback-return": Linter.RuleEntry<
		[
			Partial<{
				/**
				 * @default false
				 */
				allowImplicit: boolean;
				/**
				 * @default false
				 */
				checkForEach: boolean;
				/**
				 * @default false
				 */
				allowVoid: boolean;
			}>,
		]
	>;

	/**
	 * Rule to enforce line breaks after each array element.
	 *
	 * @since 4.0.0-rc.0
	 * @deprecated since 8.53.0.
	 * Formatting rules are being moved out of ESLint core.
	 * Please, use [`array-element-newline`](https://eslint.style/rules/array-element-newline) in [`@stylistic/eslint-plugin`](https://eslint.style).
	 * @see https://eslint.org/docs/latest/rules/array-element-newline
	 */
	"array-element-newline": Linter.RuleEntry<
		[
			| "always"
			| "never"
			| "consistent"
			| Partial<{
					/**
					 * @default true
					 */
					multiline: boolean;
					/**
					 * @default null
					 */
					minItems: number | null;
			  }>,