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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

@babel/eslint-plugin: Update rules/tests to use @babel/eslint-parser #10977

Merged
merged 7 commits into from Jan 11, 2020
Merged

@babel/eslint-plugin: Update rules/tests to use @babel/eslint-parser #10977

merged 7 commits into from Jan 11, 2020

Conversation

kaicataldo
Copy link
Member

Q A
Fixed Issues? refs #10752, #10975
Patch: Bug Fix?
Major: Breaking Change? 馃憤
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This PR builds on #10975 (we should land that first or, if folks prefer, this PR can supersede that one) and does the following:

  1. Removes the remaining unnecessary rules from @babel/eslint-plugin
  2. Uses @babel/eslint-parser for @babel/eslint-plugin's tests
  3. Start a new private packages called eslint/babel-eslint-shared-fixtures so that we can centralize shared fixtures for the ESLint packages
  4. Remove rule overrides for stage 4 features. Some of these aren't handled in the ESLint core rules yet (currently blocked by Add optional chaining聽estree/estree#204), but they will be, and so I think it's best to not duplicate throwaway work here.
  5. I've deleted all test cases that are not specifically testing the overridden behavior in each rule. I'd like to figure out a way to continue testing core rule tests using these packages, but manually managing copying over tests every release is a huge hassle and not a road I think we should go down. Note that some of these tests cases are very old and contained options that no longer exist.

This should get us back to a pretty sane state that we can use as a foundation to figure out what our maintenance strategy will be here. Excited to start figuring that stuff out!

@kaicataldo kaicataldo changed the title Fresh start @babel/eslint-plugin: Update rules/tests to use @babel/eslint-parser Jan 10, 2020
@kaicataldo kaicataldo force-pushed the fresh-start branch 2 times, most recently from 6f2e00d to 994fa62 Compare January 10, 2020 04:08
kaicataldo
kaicataldo commented Jan 10, 2020
View reviewed changes
eslint/babel-eslint-plugin/src/rules/object-curly-spacing.js
if (
node.type === "ExportNamedDeclaration" &&
node.specifiers.length === 1 &&
node.specifiers[0].type === "ExportDefaultSpecifier"
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 code change is to allow the core rule to handle exportNamespaceFrom, since it's stage 4.

kaicataldo
kaicataldo commented Jan 10, 2020
View reviewed changes
eslint/babel-eslint-plugin/src/rules/semi.js
@@ -102,21 +103,3 @@ const semiRuleWithClassProperty = ruleComposer.joinReports([
},
}),
]);

export default ruleComposer.filterReports(
Copy link
Member Author

Choose a reason for hiding this comment

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

Async iterators should be handled by ESLint core

kaicataldo
kaicataldo commented Jan 10, 2020
View reviewed changes
eslint/babel-eslint-shared-fixtures/package.json
"description": "Shared fixtures for testing @babel/eslint-* packages",
"license": "MIT",
"private": true,
"dependencies": {
Copy link
Member Author

Choose a reason for hiding this comment

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

Wanted to check to make sure this seemed like the best way to share these configs.

Copy link
Member

Choose a reason for hiding this comment

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

We don't do anything like this currently, but I think that it's ok.

nicolo-ribaudo
nicolo-ribaudo reviewed Jan 10, 2020
View reviewed changes
eslint/babel-eslint-plugin/src/rules/semi.js

const isUnnecessarySemicolon = (context, lastToken) => {
function isUnnecessarySemicolon(context, lastToken) {
Copy link
Member

Choose a reason for hiding this comment

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

Fwiw, I love this change 馃槀

eslint/babel-eslint-parser/package.json Outdated
@@ -29,22 +29,8 @@
"semver": "^6.3.0"
},
"devDependencies": {
"@babel/eslint-shared-fixtures": "0.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

I think that lerna will update 0.0.0. Maybe we could use * instead?

@nicolo-ribaudo
Copy link
Member

Should we also update the peerDependency on eslint, since we now are assuming that those rules don't need to be fixed?

@kaicataldo
Copy link
Member Author

Thanks for the reviews!

Should we also update the peerDependency on eslint, since we now are assuming that those rules don't need to be fixed?

I thought I did this, but maybe I'm misunderstanding. Mind pointing me to which lines you're talking about?

@nicolo-ribaudo
Copy link
Member

Nevermind, I missed it somehow 馃槄

@kaicataldo
Copy link
Member Author

Thanks again for the reviews! Any other concerns with this?

@nicolo-ribaudo nicolo-ribaudo merged commit d8e6219 into babel:master Jan 11, 2020
@kaicataldo kaicataldo deleted the fresh-start branch January 11, 2020 20:57
@kaicataldo kaicataldo mentioned this pull request Jan 14, 2020
Closed
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Apr 12, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@existentialism existentialism existentialism approved these changes

@JLHwung JLHwung JLHwung approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees
No one assigned
Labels
area: eslint area: tests outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kaicataldo @nicolo-ribaudo @existentialism @JLHwung