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

docs: Custom rule & plugin tutorial #17024

Merged
merged 40 commits into from Jun 12, 2023
Merged

Conversation

bpmutter
Copy link
Contributor

@bpmutter bpmutter commented Mar 27, 2023

Prerequisites checklist

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

[x] 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
[ ] Other, please explain:

What changes did you make? (Give an overview)

Added tutorial to documentation explaining how to create a custom rule and publish it in a plugin.

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

  • are there any best practices not being followed here?
  • is additional documentation needed for anything?

Resolves #16940

@eslint-github-bot eslint-github-bot bot added the documentation Relates to ESLint's documentation label Mar 27, 2023
@netlify
Copy link

netlify bot commented Mar 27, 2023

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit bef27bc
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/64738b0e429724000756ed34
😎 Deploy Preview https://deploy-preview-17024--docs-eslint.netlify.app/extend/custom-rule-tutorial
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@bpmutter bpmutter marked this pull request as ready for review March 28, 2023 01:49
@bpmutter bpmutter requested a review from a team as a code owner March 28, 2023 01:49
@amareshsm amareshsm added the accepted There is consensus among the team that this change meets the criteria for inclusion label Mar 28, 2023
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.

Can you rebase onto main? There are a bunch of commits and other unrelated changes that seem to be in this PR, which makes it difficult to know where to focus.

docs/_custom-rule-tutorial-code/.eslintrc.js Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-plugin-tutorial.md Outdated Show resolved Hide resolved
@bpmutter bpmutter force-pushed the custom_rule_tutorial branch 2 times, most recently from 845532a to 0f5b843 Compare March 29, 2023 01:57
@bpmutter
Copy link
Contributor Author

Can you rebase onto main? There are a bunch of commits and other unrelated changes that seem to be in this PR, which makes it difficult to know where to focus.

@nzakas sorry for confusion here. not sure what was going on w my git. had to do some funky troubleshooting to resolve. but state of the PR now should be good to review now

@bpmutter bpmutter requested a review from nzakas March 30, 2023 01:41
};
```

To learn more about rule metadata, refer to [Rule Structure](custom-rules#rule-structure).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

No worries. We'll merge that PR first and then we can rebase this one.

@bpmutter
Copy link
Contributor Author

bpmutter commented Apr 2, 2023

when updating the tutorial i got the following error on the pre-commit hook:

➜  eslint git:(custom_rule_tutorial) ✗ git commit -m"update tutorial" 
 > running pre-commit hook: lint-staged
✔ Preparing...
⚠ Running tasks...
  ❯ Running tasks for *.js
    ✖ eslint --fix [FAILED]
  ✔ Running tasks for *.md
  ↓ No staged files match lib/rules/*.js [SKIPPED]
  ↓ No staged files match docs/src/rules/*.md [SKIPPED]
  ↓ No staged files match docs/**/*.svg [SKIPPED]
↓ Skipped because of errors from tasks. [SKIPPED]
✔ Reverting to original state because of errors...
✔ Cleaning up...

✖ eslint --fix:

Oops! Something went wrong! :(

ESLint: 8.37.0

Error: Cannot find module '/Users/benperlmutter1/Library/Mobile Documents/com~apple~CloudDocs/eslint/node_modules/eslint-rule-composer/lib/rule-composer.js'. Please verify that the package.json has a valid "main" entry
    at tryPackage (node:internal/modules/cjs/loader:436:19)
    at Module._findPath (node:internal/modules/cjs/loader:678:18)
    at Module._resolveFilename (node:internal/modules/cjs/loader:1061:27)
    at Module._load (node:internal/modules/cjs/loader:920:27)
    at Module.require (node:internal/modules/cjs/loader:1141:19)
    at require (node:internal/modules/cjs/helpers:110:18)
    at Object.<anonymous> (/Users/benperlmutter1/Library/Mobile Documents/com~apple~CloudDocs/eslint/tools/internal-rules/multiline-comment-style.js:8:22)
    at Module._compile (node:internal/modules/cjs/loader:1254:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
    at Module.load (node:internal/modules/cjs/loader:1117:32)

the error seems to not be related to my changes so i bypassed the commit hook with the --no-verify flag in order to push the changes up for review.

do any of the reviewers have an idea how to fix this error?

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.

Thanks for putting this together! This will be really useful to people once complete.

@@ -0,0 +1,4 @@
"use strict";
Copy link
Member

Choose a reason for hiding this comment

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

Because we are using these as examples, I think it would be helpful to have a @fileoverview comment at the top explaining what each file does. Most of the files in the eslint repo already have this, so you can copy-paste from there.

Copy link

Choose a reason for hiding this comment

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

Because we are using these as examples, I think it would be helpful to have a @fileoverview comment at the top explaining what each file does. Most of the files in the eslint repo already have this, so you can copy-paste from there.

I like this suggestion.

docs/_custom-rule-tutorial-code/eslint.config.js Outdated Show resolved Hide resolved
docs/_custom-rule-tutorial-code/foo-bar.js Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-plugin-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-plugin-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-plugin-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-plugin-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-plugin-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-plugin-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-plugin-tutorial.md Outdated Show resolved Hide resolved
@nzakas
Copy link
Member

nzakas commented Apr 3, 2023

@bpmutter try running npm install again. It may be that dependencies changed since the last time.

@bpmutter bpmutter requested a review from nzakas April 7, 2023 02:46
docs/src/extend/index.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-tutorial.md Show resolved Hide resolved
docs/src/extend/custom-rule-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-tutorial.md Outdated Show resolved Hide resolved
Copy link
Contributor

@moniuch moniuch left a comment

Choose a reason for hiding this comment

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

First off, as the OP of 16854, big thanks for addressing this ticket ❤️ This is a huuuge improvement for developers!

Besides some comments I have left (subject to a native English speaker's review and approval), I would suggest the following followup PRs:

  1. The guide assumes writing eslint plugin in JS. Would be good to see some advice on authoring in TS (eg. recommended packages, @types, other comments from experienced devs)

  2. Would be good to provide insights on creating a plugin without having to publish it as a package, but instead aliasing the directory in package.json. What are pros and cons of hosting a plugin in a local directory within a project? In some enterprise settings, npm publishing process might be seen as an overhead that will stop devs from actually creating a plugin.

docs/src/extend/custom-rule-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-tutorial.md Outdated Show resolved Hide resolved
ecmaVersion: "latest",
},
// Using the eslint-plugin-foo-bar plugin downloaded from npm
plugins: {"example": eslintPluginExample},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
plugins: {"example": eslintPluginExample},
// By design, whatever follows `eslint-plugin-` in the package name,
// becomes the short plugin name to be used in eslint config files, here: "example"
plugins: {"example": eslintPluginExample},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@moniuch we are actually defining the name as 'example' here (not a shorthand), as this is following the new config format.

docs/src/extend/custom-rule-tutorial.md Outdated Show resolved Hide resolved
@nzakas nzakas closed this Apr 20, 2023
@nzakas nzakas reopened this Apr 20, 2023
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 20, 2023

CLA Missing ID CLA Not Signed

@nzakas
Copy link
Member

nzakas commented Apr 20, 2023

@moniuch Thanks for the feedback.

The guide assumes writing eslint plugin in JS. Would be good to see some advice on authoring in TS (eg. recommended packages, https://github.com/types, other comments from experienced devs)

We are keeping our documentation specific to JavaScript, as a guide for TypeScript would largely be duplicated content. Since all JavaScript is also valid TypeScript, having a JavaScript guide works for everyone.

Would be good to provide insights on creating a plugin without having to publish it as a package, but instead aliasing the directory in package.json. What are pros and cons of hosting a plugin in a local directory within a project? In some enterprise settings, npm publishing process might be seen as an overhead that will stop devs from actually creating a plugin.

I agree, this would be nice to add an as alternative to publishing to npm.

@moniuch
Copy link
Contributor

moniuch commented Apr 20, 2023

@nzakas

Since all JavaScript is also valid TypeScript, having a JavaScript guide works for everyone.

Well, I wouldn't dare to disagree on that ;-) What I meant was not an entire parallel version for TypeScript, but rather a chapter about some useful supporting libraries out there like @typescript-eslint/utils (AST_NODE_TYPES, ESLintUtils, TSESTree), maybe a starter repo for TS, or pointers to plugin projects that are known to be using TypeScript, or code hints such as that nodes are typed using discriminated union types, so it is necessary to first narrow down the type before accessing a property. (...and probably much more than that - I've only started my journey here, so who knows what other things ahead I will need to discover on my own).

@bpmutter
Copy link
Contributor Author

I agree, this would be nice to add an as alternative to publishing to npm.

@nzakas and @moniuch, refactored to include local only plugin as a precursor to publishing to npm (added in new step 8 w step 10 refactored)

@bpmutter
Copy link
Contributor Author

@nzakas ready for re-review

@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@bpmutter
Copy link
Contributor Author

bpmutter commented Jun 8, 2023

@nzakas ready for merge i believe

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

Just one thing that should be fixed before merging.

docs/src/extend/custom-rule-tutorial.md Show resolved Hide resolved
Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM. A few minor edits. Thank you for all your hard work and for bearing with us all of these back-and-forth reviews.

docs/src/extend/custom-rule-tutorial.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rule-tutorial.md Outdated Show resolved Hide resolved
@snitin315
Copy link
Contributor

Also the verify files check seems unhappy, can you look into that?

@bpmutter
Copy link
Contributor Author

Also the verify files check seems unhappy, can you look into that?

it's b/c of the reversion of the custom rules page. once we resubmit that, this will be fixed

Co-authored-by: Nitin Kumar <snitin315@gmail.com>
@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@mdjermanovic
Copy link
Member

Also the verify files check seems unhappy, can you look into that?

it's b/c of the reversion of the custom rules page. once we resubmit that, this will be fixed

I'll close-reopen this now to run the check again.

@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

Copy link
Member

@mdjermanovic mdjermanovic 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!

@mdjermanovic
Copy link
Member

I didn't merge this because I'm not sure if we should wait until the EasyCLA problem is resolved.

@nzakas
Copy link
Member

nzakas commented Jun 12, 2023

Let me try one more time.

@nzakas nzakas closed this Jun 12, 2023
@nzakas nzakas reopened this Jun 12, 2023
@nzakas
Copy link
Member

nzakas commented Jun 12, 2023

There we go. I got an email this morning saying that the bug in EasyCLA was fixed, so we should be good to go across the board.

@nzakas nzakas merged commit e0cf0d8 into eslint:main Jun 12, 2023
40 of 41 checks passed
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 10, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Request: Custom Rule tutorial
9 participants