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.

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

feat: add name to eslint configs #18289

Merged
merged 6 commits into from Apr 18, 2024

Conversation

aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented Apr 8, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ x] Other, please explain:

What changes did you make? (Give an overview)

added name to config, per https://eslint.org/docs/head/use/configure/configuration-files#configuration-naming-conventions

Is there anything you'd like reviewers to focus on?

suggestions for the naming?

@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Apr 8, 2024
Copy link

netlify bot commented Apr 8, 2024

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 7d93592
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/661f31ae13bd53000805fef6

@mdjermanovic
Copy link
Member

Looks like there are merge conflicts in packages/eslint-config-eslint/base.js now.

@aladdin-add aladdin-add changed the title feat: add name to exported configs feat: add name to eslint configs Apr 15, 2024
@aladdin-add aladdin-add force-pushed the feat/add-name-to-configs branch 2 times, most recently from 15af7ad to 7e93954 Compare April 15, 2024 03:22
@aladdin-add aladdin-add marked this pull request as ready for review April 15, 2024 03:22
@aladdin-add aladdin-add requested a review from a team as a code owner April 15, 2024 03:22
nzakas
nzakas previously approved these changes Apr 15, 2024
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Would like another review before merging.

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Apr 16, 2024
@mdjermanovic
Copy link
Member

There's a merge conflict in tests/lib/cli.js now.

@@ -1771,7 +1771,7 @@ describe("cli", () => {
getFixturePath("globals-node.js")
];

await cli.execute(`--no-eslintrc --config ./packages/js/src/configs/eslint-recommended.js --no-ignore ${files.join(" ")}`, null, false);
await cli.execute(`--no-eslintrc --config ./tests/fixtures/config-file/js/.eslintrc.js --no-ignore ${files.join(" ")}`, null, false);
Copy link
Member Author

Choose a reason for hiding this comment

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

name is not allowed in eslintrc mode. however, js.configs.recommended was used here, so I updated the test to avoid failing.

@aladdin-add aladdin-add marked this pull request as ready for review April 17, 2024 02:22
Copy link
Member

@fasttime fasttime left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@fasttime fasttime merged commit 4d11e56 into eslint:main Apr 18, 2024
18 checks passed
@aladdin-add aladdin-add deleted the feat/add-name-to-configs branch April 19, 2024 02:03
@fasttime
Copy link
Member

Part of the changes in this PR have been reverted in #18368 because they were causing some unit tests to fail. The tests started to fail only after upgrading the @eslint/js dependency in eslint's package.json.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

4 participants