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: Integration section and tutorial #17132
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
12a09a2
to
8b4b4a5
Compare
Errors with verification of files:
|
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.
I think the overall approach is helpful, but some of the naming and details are incorrect. We can keep iterating on those details.
Overall, I think this is a good way to introduce how to use the Node.js API in a way that will be useful to most folks.
- **Code review tools**: Integrating ESLint with code review tools can help automate the process of identifying potential issues in the codebase. | ||
|
||
- **Learning platforms**: If you are developing a learning platform or coding tutorial, integrating ESLint can provide real-time feedback to users as they learn JavaScript, helping them improve their coding skills and learn best practices. | ||
|
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.
I'd also add plugins for code-related tools like Webpack and Jest.
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.
adding a line for 'developer tools'
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.
I think we're still missing the Webpack/Jest plugin use case. In that case you're not creating a new tool but rather extending an existing tool. I think we need to be more explicit about this use case.
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.
updated the bullet point. lmk what you think of the update
To lint a file, use the `lintFiles` method of the `ESLint` instance: | ||
|
||
The `filePaths` argument passed to `ESLint#lintFiles()` can be a string or an array of strings, representing the file path(s) you want to lint. | ||
|
||
To format the linting results for better readability, use the `outputFixes` and `getErrorResults` methods: |
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.
This text feels a bit disjointed to me and disconnected from the following code example. Can you expand this to talk a bit more about what each method does? For instance, lintFiles searches the filesystem and the file paths can be globs or filenames.
Also, outputFixes isn't for readability, it's the method that outputs fixes back to disk. getErrorResults filters out all warnings.
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.
updated copy here
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
Not stale. Waiting for @nzakas's review. |
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.
This is looking good! Just a few suggestions throughout.
- **Code review tools**: Integrating ESLint with code review tools can help automate the process of identifying potential issues in the codebase. | ||
|
||
- **Learning platforms**: If you are developing a learning platform or coding tutorial, integrating ESLint can provide real-time feedback to users as they learn JavaScript, helping them improve their coding skills and learn best practices. | ||
|
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.
I think we're still missing the Webpack/Jest plugin use case. In that case you're not creating a new tool but rather extending an existing tool. I think we need to be more explicit about this use case.
npm init -y | ||
``` | ||
|
||
Install the `eslint` package as a dependency: |
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.
It may be worth calling out that the package is installed as a dependency and not a dev dependency for this use case.
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.
updated
|
||
## View the Tutorial Code | ||
|
||
You can view the annotated source code for the tutorial [here](https://github.com/eslint/eslint/tree/main/docs/_integration-tutorial-code). |
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.
Rather than creating a top-level directory under docs
, maybe we should have docs/_examples
for all examples and then a subdirectory under docs/_examples
for the integration example code?
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.
moved
|
|
@nzakas i think this might be something you have to do on your end. on these 2 commits which the bot has flagged, you're coauthor b/c i accepted your suggestion |
|
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. Can you rebase on top of main to pull in our updated docs license?
I think the CLA bot might have a bug. The email address on my suggestion is the same one I always use and it's definitely tied to my GitHub account. I've opened a bug with EasyCLA. |
Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
59e43fc
to
1716a0a
Compare
|
rebased! |
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.
Looks good overall. A few suggestions.
Co-authored-by: Nitin Kumar <snitin315@gmail.com>
|
/easycla |
@mdjermanovic do you want to review this before merging? |
I'll take a look now. |
"author": "", | ||
"license": "ISC", | ||
"devDependencies": { | ||
"eslint": "^8.39.0", |
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.
Per the tutorial, eslint
should be in dependencies
?
// Apply automatic fixes and output fixed code | ||
await ESLint.outputFixes(results); |
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.
If we want to autofix files, then the constructor must be called with fix: true
. Otherwise (with the default fix: false
), this step wouldn't be doing anything.
if (results.length) { | ||
console.log("Linting errors found!"); |
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.
ESLint#lintFiles()
returns an array of LintResult
objects, one for each file that was linted regardless of whether the file had any linting errors, so result.length > 0
doesn't mean that there were linting errors found.
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.
adjusting. pls lmk what you think of the new approach
docs/_examples/integration-tutorial-code/example-eslint-integration.test.js
Outdated
Show resolved
Hide resolved
* @fileoverview Example data to lint using ESLint. This file contains a variety of errors. | ||
* @author Ben Perlmutter | ||
*/ | ||
v// 'var' should be replaced with 'const' or 'let' (no-var from eslint:recommended) |
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-var
is not in eslint:recommended
and it isn't enabled in the hardcoded configuration inside example-eslint-integration
.
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.
hmm, i worked on this so long ago that i don't remember context on this. deleting this examples from the file.
// 'result' is assigned a value but never used (no-unused-vars from custom rules) | ||
const result = add(x, 5); | ||
|
||
// Expected indentation of 2 spaces but found 4 (indent from eslint:recommended) |
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.
Same as above, indent
is not in eslint:recommended
and it isn't enabled in the hardcoded configuration inside example-eslint-integration
.
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.
same comment as above. don't remember context. deleting the example
"eslint-config-airbnb-base": "^15.0.0", | ||
"eslint-plugin-import": "^2.27.5" |
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.
Do we need these in the example project?
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.
removing.
i had these and the .eslintrc file b/c i was using it to lint the code i was writing
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.
I think this file isn't used in any way, so we could remove it?
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@mdjermanovic ready for re-review @nzakas accidentally re-requested review. sorry about that. no action necessary right now |
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!
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://togithub.com/eslint/eslint)) | devDependencies | minor | [`8.43.0` -> `8.44.0`](https://renovatebot.com/diffs/npm/eslint/8.43.0/8.44.0) | --- ### Release Notes <details> <summary>eslint/eslint (eslint)</summary> ### [`v8.44.0`](https://togithub.com/eslint/eslint/releases/tag/v8.44.0) [Compare Source](https://togithub.com/eslint/eslint/compare/v8.43.0...v8.44.0) #### Features - [`1766771`](https://togithub.com/eslint/eslint/commit/176677180a4a1209fc192771521c9192e1f67578) feat: add `es2023` and `es2024` environments (#​1[eslint/eslint#17328)) (Milos Djermanovic) - [`4c50400`](https://togithub.com/eslint/eslint/commit/4c5040022639ae804c15b366afc6e64982bd8ae3) feat: add `ecmaVersion: 2024`, regexp `v` flag parsing (#​1[eslint/eslint#17324)) (Milos Djermanovic) - [`4d411e4`](https://togithub.com/eslint/eslint/commit/4d411e4c7063274d6d346f1b7ee46f7575d0bbd2) feat: add ternaryOperandBinaryExpressions option to no-extra-parens rule (#​1[eslint/eslint#17270)) (Percy Ma) - [`c8b1f4d`](https://togithub.com/eslint/eslint/commit/c8b1f4d61a256727755d561bf53f889b6cd712e0) feat: Move `parserServices` to `SourceCode` (#​1[eslint/eslint#17311)) (Milos Djermanovic) - [`ef6e24e`](https://togithub.com/eslint/eslint/commit/ef6e24e42670f321d996948623846d9caaedac99) feat: treat unknown nodes as having the lowest precedence (#​1[eslint/eslint#17302)) (Brad Zacher) - [`1866e1d`](https://togithub.com/eslint/eslint/commit/1866e1df6175e4ba0ae4a0d88dc3c956bb310035) feat: allow flat config files to export a Promise (#​1[eslint/eslint#17301)) (Milos Djermanovic) #### Bug Fixes - [`a36bcb6`](https://togithub.com/eslint/eslint/commit/a36bcb67f26be42c794797d0cc9948b9cfd4ff71) fix: no-unused-vars false positive with logical assignment operators (#​1[eslint/eslint#17320)) (Gweesin Chan) - [`7620b89`](https://togithub.com/eslint/eslint/commit/7620b891e81c234f30f9dbcceb64a05fd0dde65e) fix: Remove `no-unused-labels` autofix before potential directives (#​1[eslint/eslint#17314)) (Francesco Trotta) - [`391ed38`](https://togithub.com/eslint/eslint/commit/391ed38b09bd1a3abe85db65b8fcda980ab3d6f4) fix: Remove `no-extra-semi` autofix before potential directives (#​1[eslint/eslint#17297)) (Francesco Trotta) #### Documentation - [`526e911`](https://togithub.com/eslint/eslint/commit/526e91106e6fe101578e9478a9d7f4844d4f72ac) docs: resubmit pr 17115 doc changes (#​1[eslint/eslint#17291)) (唯然) - [`e1314bf`](https://togithub.com/eslint/eslint/commit/e1314bf85a52bb0d05b1c9ca3b4c1732bae22172) docs: Integration section and tutorial (#​1[eslint/eslint#17132)) (Ben Perlmutter) - [`19a8c5d`](https://togithub.com/eslint/eslint/commit/19a8c5d84596a9f7f2aa428c1696ba86daf854e6) docs: Update README (GitHub Actions Bot) #### Chores - [`49e46ed`](https://togithub.com/eslint/eslint/commit/49e46edf3c8dc71d691a97fc33b63ed80ae0db0c) chore: upgrade [@​eslint/js](https://togithub.com/eslint/js)[@​8](https://togithub.com/8).44.0 (#​1[eslint/eslint#17329)) (Milos Djermanovic) - [`a1cb642`](https://togithub.com/eslint/eslint/commit/a1cb6421f9d185901cd99e5f696e912226ef6632) chore: package.json update for [@​eslint/js](https://togithub.com/eslint/js) release (ESLint Jenkins) - [`840a264`](https://togithub.com/eslint/eslint/commit/840a26462bbf6c27c52c01b85ee2018062157951) test: More test cases for no-case-declarations (#​1[eslint/eslint#17315)) (Elian Cordoba) - [`e6e74f9`](https://togithub.com/eslint/eslint/commit/e6e74f9eef0448129dd4775628aba554a2d8c8c9) chore: package.json update for eslint-config-eslint release (ESLint Jenkins) - [`eb3d794`](https://togithub.com/eslint/eslint/commit/eb3d7946e1e9f70254008744dba2397aaa730114) chore: upgrade semver@7.5.3 (#​1[eslint/eslint#17323)) (Ziyad El Abid) - [`cf88439`](https://togithub.com/eslint/eslint/commit/cf884390ad8071d88eae05df9321100f1770363d) chore: upgrade optionator@0.9.3 (#​1[eslint/eslint#17319)) (Milos Djermanovic) - [`9718a97`](https://togithub.com/eslint/eslint/commit/9718a9781d69d2c40b68c631aed97700b32c0082) refactor: remove unnecessary code in `flat-eslint.js` (#​1[eslint/eslint#17308)) (Milos Djermanovic) - [`f82e56e`](https://togithub.com/eslint/eslint/commit/f82e56e9acfb9562ece76441472d5657d7d5e296) perf: various performance improvements (#​1[eslint/eslint#17135)) (moonlightaria) - [`da81e66`](https://togithub.com/eslint/eslint/commit/da81e66e22b4f3d3fe292cf70c388753304deaad) chore: update eslint-plugin-jsdoc to 46.2.5 (#​1[eslint/eslint#17245)) (唯然) - [`b991640`](https://togithub.com/eslint/eslint/commit/b991640176d5dce4750f7cc71c56cd6f284c882f) chore: switch eslint-config-eslint to the flat format (#​1[eslint/eslint#17247)) (唯然) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xNTUuMCIsInVwZGF0ZWRJblZlciI6IjM1LjE1OS4zIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: Renovate Bot <bot@renovateapp.com>
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)
Is there anything you'd like reviewers to focus on?
Resolves #17134
Resolves #17133