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

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

Merged
merged 7 commits into from Jan 11, 2020

Conversation

@kaicataldo
Copy link
Member

kaicataldo commented Jan 10, 2020

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 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 kaicataldo:fresh-start branch from 781a102 to d89cf03 Jan 10, 2020
@kaicataldo kaicataldo force-pushed the kaicataldo:fresh-start branch 2 times, most recently from 6f2e00d to 994fa62 Jan 10, 2020
if (
node.type === "ExportNamedDeclaration" &&
node.specifiers.length === 1 &&
node.specifiers[0].type === "ExportDefaultSpecifier"

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Jan 10, 2020

Author Member

This code change is to allow the core rule to handle exportNamespaceFrom, since it's stage 4.

@@ -102,21 +103,3 @@ const semiRuleWithClassProperty = ruleComposer.joinReports([
},
}),
]);

export default ruleComposer.filterReports(

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Jan 10, 2020

Author Member

Async iterators should be handled by ESLint core

"description": "Shared fixtures for testing @babel/eslint-* packages",
"license": "MIT",
"private": true,
"dependencies": {

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Jan 10, 2020

Author Member

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

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 10, 2020

Member

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

@kaicataldo kaicataldo force-pushed the kaicataldo:fresh-start branch from 7aa4dba to 5292fd9 Jan 10, 2020

const isUnnecessarySemicolon = (context, lastToken) => {
function isUnnecessarySemicolon(context, lastToken) {

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 10, 2020

Member

Fwiw, I love this change 馃槀

@@ -29,22 +29,8 @@
"semver": "^6.3.0"
},
"devDependencies": {
"@babel/eslint-shared-fixtures": "0.0.0",

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 10, 2020

Member

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

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Jan 10, 2020

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

@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Jan 10, 2020

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?

@kaicataldo kaicataldo force-pushed the kaicataldo:fresh-start branch from 5292fd9 to 2db04ba Jan 10, 2020
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Jan 10, 2020

Nevermind, I missed it somehow 馃槄

@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Jan 11, 2020

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

@nicolo-ribaudo nicolo-ribaudo merged commit d8e6219 into babel:master Jan 11, 2020
4 of 5 checks passed
4 of 5 checks passed
build (13.x)
Details
test262 Workflow: test262
Details
Travis CI - Pull Request Build Passed
Details
build-standalone Workflow: build-standalone
Details
e2e Workflow: e2e
Details
@kaicataldo kaicataldo deleted the kaicataldo:fresh-start branch Jan 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can鈥檛 perform that action at this time.