Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: use eslint-plugin-eslint-plugin flat configs #17204

Merged
merged 6 commits into from Jun 7, 2023
Merged

chore: use eslint-plugin-eslint-plugin flat configs #17204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3a30c6e
chore: use eslint-plugin-eslint-plugin flat configs
mdjermanovic May 20, 2023
ee52efc
fix bin tests
mdjermanovic May 22, 2023
cc9019a
fix cli tests
mdjermanovic May 31, 2023
21a6225
fix flat-eslint tests
mdjermanovic Jun 1, 2023
3f8b593
fix eslint tests
mdjermanovic Jun 4, 2023
39f5786
fix cli-engine tests
mdjermanovic Jun 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 0 additions & 4 deletions .eslintrc.js
View file
Open in desktop
Expand Up @@ -96,9 +96,6 @@ module.exports = {
"plugin:eslint-plugin/rules-recommended"
],
rules: {
"eslint-plugin/no-missing-message-ids": "error",
"eslint-plugin/no-unused-message-ids": "error",
"eslint-plugin/prefer-message-ids": "error",
"eslint-plugin/prefer-placeholders": "error",
"eslint-plugin/prefer-replace-text": "error",
"eslint-plugin/report-message-format": ["error", "[^a-z].*\\.$"],
Expand All @@ -119,7 +116,6 @@ module.exports = {
"plugin:eslint-plugin/tests-recommended"
],
rules: {
"eslint-plugin/prefer-output-null": "error",
"eslint-plugin/test-case-property-ordering": "error",
"eslint-plugin/test-case-shorthand-strings": "error"
}
Expand Down
41 changes: 20 additions & 21 deletions eslint.config.js
View file
Open in desktop
Expand Up @@ -27,10 +27,12 @@

const path = require("path");
const internalPlugin = require("eslint-plugin-internal-rules");
const eslintPlugin = require("eslint-plugin-eslint-plugin");
const eslintPluginRulesRecommendedConfig = require("eslint-plugin-eslint-plugin/configs/rules-recommended");
const eslintPluginTestsRecommendedConfig = require("eslint-plugin-eslint-plugin/configs/tests-recommended");
Comment on lines +30 to +31
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you destructure these configs from require('eslint-plugin-eslint-plugin')? That will be more future-proof than reaching directly into specific files in the event that the plugin more-strictly defines its Node API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the only way to use flat configs from eslint-plugin-eslint-plugin at the moment.

https://github.com/eslint-community/eslint-plugin-eslint-plugin#eslintconfigjs-requires-eslintv8230

Configs in require('eslint-plugin-eslint-plugin').configs are in the eslintrc format.

@aladdin-add is that correct?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not blocking) @aladdin-add shouldn't we export the flat configs too? Reaching into specific files is fragile especially if we were to move things around.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are exported. The default predefined configs are eslintrc, and to not affect the use of eslintrc, I added a new directory to place the flat configs: https://github.com/eslint-community/eslint-plugin-eslint-plugin/tree/main/configs. Any better suggestions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to merge this as it correctly uses the current interface of eslint-plugin-eslint-plugin. We can update the usage if and when the interface changes.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So require('eslint-plugin-eslint-plugin').configs will always have to be in the eslintrc format for backwards compatibility, but flat configs have to be accessed by file path e.g. require("eslint-plugin-eslint-plugin/configs/rules-recommended")? Is there a cleaner, long-term plan or recommendation for supporting both config formats (instead of just having to access the newer one by file path)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So require('eslint-plugin-eslint-plugin').configs will always have to be in the eslintrc format for backwards compatibility, but flat configs have to be accessed by file path e.g. require("eslint-plugin-eslint-plugin/configs/rules-recommended")? Is there a cleaner, long-term plan or recommendation for supporting both config formats (instead of just having to access the newer one by file path)?

I think we could discuss that in #17242.

const { FlatCompat } = require("@eslint/eslintrc");
const js = require("./packages/js");
const globals = require("globals");
const merge = require("lodash.merge");

//-----------------------------------------------------------------------------
// Helpers
Expand Down Expand Up @@ -99,8 +101,7 @@ module.exports = [
},
{
plugins: {
"internal-rules": internalPlugin,
"eslint-plugin": eslintPlugin
"internal-rules": internalPlugin
},
languageOptions: {
ecmaVersion: "latest"
Expand Down Expand Up @@ -129,33 +130,31 @@ module.exports = [
{
files: ["lib/rules/*", "tools/internal-rules/*"],
ignores: ["**/index.js"],
rules: {
...eslintPlugin.configs["rules-recommended"].rules,
"eslint-plugin/no-missing-message-ids": "error",
"eslint-plugin/no-unused-message-ids": "error",
"eslint-plugin/prefer-message-ids": "error",
Comment on lines -134 to -136
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three rules are now already in the rules recommended config, so I removed them from here.

"eslint-plugin/prefer-placeholders": "error",
"eslint-plugin/prefer-replace-text": "error",
"eslint-plugin/report-message-format": ["error", "[^a-z].*\\.$"],
"eslint-plugin/require-meta-docs-description": ["error", { pattern: "^(Enforce|Require|Disallow) .+[^. ]$" }],
"internal-rules/no-invalid-meta": "error"
}
...merge({}, eslintPluginRulesRecommendedConfig, {
rules: {
"eslint-plugin/prefer-placeholders": "error",
"eslint-plugin/prefer-replace-text": "error",
"eslint-plugin/report-message-format": ["error", "[^a-z].*\\.$"],
"eslint-plugin/require-meta-docs-description": ["error", { pattern: "^(Enforce|Require|Disallow) .+[^. ]$" }],
"internal-rules/no-invalid-meta": "error"
}
})
},
{
files: ["lib/rules/*"],
ignores: ["index.js"],
ignores: ["**/index.js"],
rules: {
"eslint-plugin/require-meta-docs-url": ["error", { pattern: "https://eslint.org/docs/latest/rules/{{name}}" }]
}
},
{
files: ["tests/lib/rules/*", "tests/tools/internal-rules/*"],
rules: {
...eslintPlugin.configs["tests-recommended"].rules,
"eslint-plugin/prefer-output-null": "error",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule is now already in the tests recommended config, so I removed it from here.

"eslint-plugin/test-case-property-ordering": "error",
"eslint-plugin/test-case-shorthand-strings": "error"
}
...merge({}, eslintPluginTestsRecommendedConfig, {
rules: {
"eslint-plugin/test-case-property-ordering": "error",
"eslint-plugin/test-case-shorthand-strings": "error"
}
})
},
{
files: ["tests/**/*.js"],
Expand Down
1 change: 0 additions & 1 deletion lib/rules/logical-assignment-operators.js
View file
Open in desktop
Expand Up @@ -188,7 +188,6 @@ module.exports = {
}]
},
fixable: "code",
// eslint-disable-next-line eslint-plugin/require-meta-has-suggestions -- Does not detect conditional suggestions
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This false positive has been fixed in the new version of eslint-plugin/require-meta-has-suggestions.

hasSuggestions: true,
messages: {
assignment: "Assignment (=) can be replaced with operator assignment ({{operator}}).",
Expand Down
2 changes: 0 additions & 2 deletions lib/rules/newline-after-var.js
View file
Open in desktop
Expand Up @@ -212,7 +212,6 @@ module.exports = {
context.report({
node,
messageId: "unexpected",
data: { identifier: node.name },
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was caught by the new version of eslint-plugin/no-unused-placeholders.

fix(fixer) {
const linesBetween = sourceCode.getText().slice(lastToken.range[1], nextToken.range[0]).split(astUtils.LINEBREAK_MATCHER);

Expand All @@ -231,7 +230,6 @@ module.exports = {
context.report({
node,
messageId: "expected",
data: { identifier: node.name },
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was caught by the new version of eslint-plugin/no-unused-placeholders.

fix(fixer) {
if ((noNextLineToken ? getLastCommentLineOfBlock(nextLineNum) : lastToken.loc.end.line) === nextToken.loc.start.line) {
return fixer.insertTextBefore(nextToken, "\n\n");
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -113,7 +113,7 @@
"eslint": "file:.",
"eslint-config-eslint": "file:packages/eslint-config-eslint",
"eslint-plugin-eslint-comments": "^3.2.0",
"eslint-plugin-eslint-plugin": "^4.4.0",
"eslint-plugin-eslint-plugin": "^5.1.0",
"eslint-plugin-internal-rules": "file:tools/internal-rules",
"eslint-plugin-jsdoc": "^38.1.6",
"eslint-plugin-n": "^15.2.4",
Expand Down
8 changes: 4 additions & 4 deletions tests/bin/eslint.js
View file
Open in desktop
Expand Up @@ -204,10 +204,10 @@ describe("bin/eslint.js", () => {
});

describe("running on files", () => {
it("has exit code 0 if no linting errors occur", () => assertExitCode(runESLint(["bin/eslint.js"]), 0));
it("has exit code 0 if no linting errors occur", () => assertExitCode(runESLint(["bin/eslint.js", "--no-config-lookup"]), 0));
it("has exit code 0 if a linting warning is reported", () => assertExitCode(runESLint(["bin/eslint.js", "--no-config-lookup", "--rule", "semi: [1, never]"]), 0));
it("has exit code 1 if a linting error is reported", () => assertExitCode(runESLint(["bin/eslint.js", "--no-config-lookup", "--rule", "semi: [2, never]"]), 1));
it("has exit code 1 if a syntax error is thrown", () => assertExitCode(runESLint(["tests/fixtures/exit-on-fatal-error/fatal-error.js", "--no-ignore"]), 1));
it("has exit code 1 if a syntax error is thrown", () => assertExitCode(runESLint(["tests/fixtures/exit-on-fatal-error/fatal-error.js", "--no-config-lookup"]), 1));
});

describe("automatically fixing files", () => {
Expand Down Expand Up @@ -359,7 +359,7 @@ describe("bin/eslint.js", () => {

describe("handling crashes", () => {
it("prints the error message to stderr in the event of a crash", () => {
const child = runESLint(["--rule=no-restricted-syntax:[error, 'Invalid Selector [[[']", "Makefile.js"]);
const child = runESLint(["--rule=no-restricted-syntax:[error, 'Invalid Selector [[[']", "--no-config-lookup", "Makefile.js"]);
const exitCodeAssertion = assertExitCode(child, 2);
const outputAssertion = getOutput(child).then(output => {
const expectedSubstring = "Syntax error in selector";
Expand All @@ -372,7 +372,7 @@ describe("bin/eslint.js", () => {
});

it("prints the error message exactly once to stderr in the event of a crash", () => {
const child = runESLint(["--rule=no-restricted-syntax:[error, 'Invalid Selector [[[']", "Makefile.js"]);
const child = runESLint(["--rule=no-restricted-syntax:[error, 'Invalid Selector [[[']", "--no-config-lookup", "Makefile.js"]);
const exitCodeAssertion = assertExitCode(child, 2);
const outputAssertion = getOutput(child).then(output => {
const expectedSubstring = "Syntax error in selector";
Expand Down
5 changes: 5 additions & 0 deletions tests/fixtures/simple-valid-project/.eslintrc.js
View file
Open in desktop
@@ -0,0 +1,5 @@
module.exports = {
rules: {
quotes: ["error", "single"]
}
};
5 changes: 5 additions & 0 deletions tests/fixtures/simple-valid-project/eslint.config.js
View file
Open in desktop
@@ -0,0 +1,5 @@
module.exports = [{
rules: {
quotes: ["error", "single"]
}
}];
1 change: 1 addition & 0 deletions tests/fixtures/simple-valid-project/foo.js
View file
Open in desktop
@@ -0,0 +1 @@
var a = 'b';
1 change: 1 addition & 0 deletions tests/fixtures/simple-valid-project/src/foobar.js
View file
Open in desktop
@@ -0,0 +1 @@
var a = 'b';