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
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
a915118
to
7f4881a
Compare
There was a problem hiding this 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.
845532a
to
0f5b843
Compare
@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 |
}; | ||
``` | ||
|
||
To learn more about rule metadata, refer to [Rule Structure](custom-rules#rule-structure). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note this link is causing the CI to fail here https://github.com/eslint/eslint/actions/runs/4549420021/jobs/8021476473?pr=17024
that's b/c this link uses the internal page link that's to be added in https://github.com/eslint/eslint/pull/16906/files#diff-dc3f44ef800795b890a57c8dd95c6ce26ba8a47a4551d7a05d7a5cd9cf016597R37
There was a problem hiding this comment.
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.
when updating the tutorial i got the following error on the pre-commit hook:
the error seems to not be related to my changes so i bypassed the commit hook with the do any of the reviewers have an idea how to fix this error? |
There was a problem hiding this 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"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@bpmutter try running |
There was a problem hiding this 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:
-
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)
-
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.
ecmaVersion: "latest", | ||
}, | ||
// Using the eslint-plugin-foo-bar plugin downloaded from npm | ||
plugins: {"example": eslintPluginExample}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}, |
There was a problem hiding this comment.
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.
|
@moniuch Thanks for the feedback.
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.
I agree, this would be nice to add an as alternative to publishing to npm. |
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 |
@nzakas ready for re-review |
bef27bc
to
94530df
Compare
|
082e9d0
to
cada5cb
Compare
|
cada5cb
to
059b492
Compare
|
|
@nzakas ready for merge i believe |
There was a problem hiding this 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.
There was a problem hiding this 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.
Also the |
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>
|
I'll close-reopen this now to run the check again. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
I didn't merge this because I'm not sure if we should wait until the EasyCLA problem is resolved. |
Let me try one more time. |
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. |
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?
Resolves #16940