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

Add @babel/eslint-plugin-development-internal #11376

Merged
merged 14 commits into from Jun 22, 2020
Merged

Add @babel/eslint-plugin-development-internal #11376

merged 14 commits into from Jun 22, 2020

Conversation

kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Apr 4, 2020 •
edited by gitpod-io bot

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? 👍
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

As discussed, this PR implements a custom ESLint rule that enforces using error messages imported from a specific, configured module.

I still need to add a README and want to add more test cases, but figured I should get a WIP PR up so that I can get some early feedback. Comments/open questions:

  1. Does this API make sense? Should we also add an option that allows you to enforce specific named imports as well?
  2. Does it make sense to create a new plugin for this? Seemed like the best way to have custom rules in the repo until Config File Simplification eslint/rfcs#9 is implemented (which is still a ways out because we're working on ESLint v7).
  3. Thoughts on the name? Got a better idea? :)
  4. I generally believe in making rules as small as possible (i.e. checking one thing) and so I limited this rule to checking that the error message comes from the given errorModule. We could potentially add more validation, but in this case I think Flow covers our bases.
  5. I moved the RuleTester utility into the shared fixtures package.

@kaicataldo kaicataldo changed the title Eslint plugin internal Add @babel/eslint-plugin-internal Apr 4, 2020
@kaicataldo kaicataldo force-pushed the eslint-plugin-internal branch 6 times, most recently from 18ab35d to 9f3ef4d Compare April 4, 2020 03:48
@nicolo-ribaudo nicolo-ribaudo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Apr 5, 2020
nicolo-ribaudo
nicolo-ribaudo reviewed Apr 5, 2020
View reviewed changes
eslint/babel-eslint-plugin-internal/src/rules/dry-error-messages.js Outdated Show resolved Hide resolved
eslint/babel-eslint-plugin-internal/src/rules/dry-error-messages.js Outdated Show resolved Hide resolved
eslint/babel-eslint-plugin-internal/src/rules/dry-error-messages.js Outdated

export default {
meta: {
type: "suggestion",
Copy link
Member

Choose a reason for hiding this comment

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

Does suggestion make CI fail? (I'd like it to do so)

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, "suggestion" is referring to what type of rule this is (i.e. it enforces a code style choice rather than a possible code error). It can still be configured as "off", "warning", or "error" in our config :)

eslint/babel-eslint-plugin-internal/package.json Outdated
@@ -0,0 +1,36 @@
{
"name": "@babel/eslint-plugin-internal",
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'd prefer to call this @babel/eslint-plugin-development-internal, since it's more similar to @babel/eslint-plugin-development than to @babel/eslint-plugin.

@nicolo-ribaudo
Copy link
Member

Good job! I think that this rule is ok as-is; if we need to enforce more things we can always update it/create other rules.

@kaicataldo kaicataldo changed the title Add @babel/eslint-plugin-internal Add @babel/eslint-plugin-development-internal Apr 6, 2020
JLHwung
JLHwung approved these changes Apr 7, 2020
View reviewed changes
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

TIL awesome AST selectors.

@nicolo-ribaudo
Copy link
Member

@kaicataldo Is this still "WIP"?

@kaicataldo
Copy link
Member Author

I was planning to add a readme as well as enable the rule, but I’m also fine with doing that in a follow up PR since folks already reviewed this.

@existentialism
Copy link
Member

@kaicataldo lets add the readme and enable it

@nicolo-ribaudo nicolo-ribaudo marked this pull request as draft April 9, 2020 18:55
@nicolo-ribaudo
Copy link
Member

GitHub now allows converting WIP PRs to Draft PRs 😄

@kaicataldo
Copy link
Member Author

Apologies for the delay! Will be getting to this in the next day or two now that I'm healthy again.

@kaicataldo
Copy link
Member Author

kaicataldo commented May 1, 2020 •
edited

Is there a way to include this in the top level package.json before we publish the package? It doesn't look like we do that with anything else at the moment (though I think it would be nice to do with @babel/eslint-parser as well!).

@nicolo-ribaudo
Copy link
Member

If just specifying * as the version doesn't work you can try "@babel/eslint-plugin-development-internal": "link:./eslint/babel-eslint-plugin-development-internal"

@kaicataldo kaicataldo force-pushed the eslint-plugin-internal branch 2 times, most recently from 5506f49 to 62817aa Compare May 12, 2020 20:39
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5506f49:

Sandbox Source
busy-panini-flbgz Configuration
naughty-hellman-p6xog Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented May 12, 2020 •
edited

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/24389/

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 12, 2020 •
edited

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1f885a3:

Sandbox Source
dreamy-elbakyan-gkdo7 Configuration
dreamy-brattain-pqz0h Configuration

@kaicataldo
Copy link
Member Author

Was hoping to land this before I go on vacation, but please feel free to merge/fix up if you'd like to before I get back. Otherwise, I'll follow up when I return!

@kaicataldo
Copy link
Member Author

This is ready for final review.

@kaicataldo kaicataldo changed the base branch from master to main June 22, 2020 22:44
@kaicataldo kaicataldo force-pushed the eslint-plugin-internal branch 3 times, most recently from 5ba8da2 to bc991ac Compare June 22, 2020 23:04
@kaicataldo kaicataldo merged commit 75c2300 into babel:main Jun 22, 2020
@kaicataldo kaicataldo deleted the eslint-plugin-internal branch June 22, 2020 23:43
@jayaddison jayaddison mentioned this pull request Jul 3, 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 Sep 22, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JLHwung JLHwung JLHwung approved these changes

@existentialism existentialism existentialism approved these changes

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

Assignees
No one assigned
Labels
area: eslint outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kaicataldo @nicolo-ribaudo @existentialism @babel-bot @JLHwung