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: Integration section and tutorial #17132

Merged
merged 19 commits into from Jun 20, 2023

Conversation

bpmutter
Copy link
Contributor

@bpmutter bpmutter commented Apr 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)

  • Add Integrate with ESLint section to docs
  • create an integrations with Node.js API tutorial

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

Resolves #17134
Resolves #17133

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

netlify bot commented Apr 27, 2023

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 941d689
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6462e25a962de20008a0d873
😎 Deploy Preview https://deploy-preview-17132--docs-eslint.netlify.app
📱 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.

@ollie-iterators
Copy link

Errors with verification of files:

  • docs/src/integrate/integration-tutorial.md: 16: MD004/ul-style Unordered list style [Expected: asterisk; Actual: dash]
  • docs/src/integrate/integration-tutorial.md: 18: MD004/ul-style Unordered list style [Expected: asterisk; Actual: dash]
  • docs/src/integrate/integration-tutorial.md: 20: MD004/ul-style Unordered list style [Expected: asterisk; Actual: dash]
  • docs/src/integrate/integration-tutorial.md: 22: MD004/ul-style Unordered list style [Expected: asterisk; Actual: dash]
  • docs/src/integrate/integration-tutorial.md: 24: MD004/ul-style Unordered list style [Expected: asterisk; Actual: dash]
  • docs/src/integrate/integration-tutorial.md: 36: MD004/ul-style Unordered list style [Expected: asterisk; Actual: dash]
  • docs/src/integrate/integration-tutorial.md: 37: MD004/ul-style Unordered list style [Expected: asterisk; Actual: dash]
  • docs/src/integrate/integration-tutorial.md: 38: MD004/ul-style Unordered list style [Expected: asterisk; Actual: dash]
  • docs/src/integrate/integration-tutorial.md: 69: MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
  • docs/src/integrate/integration-tutorial.md: 137: MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
  • docs/src/integrate/integration-tutorial.md: 252: MD047/single-trailing-newline Files should end with a single newline character

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.

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.

docs/src/integrate/integration-tutorial.md Outdated Show resolved Hide resolved
docs/src/integrate/integration-tutorial.md Outdated Show resolved Hide resolved
- **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.

Copy link
Member

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.

Copy link
Contributor Author

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'

Copy link
Member

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.

Copy link
Contributor Author

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

Comment on lines 89 to 93
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:
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated copy here

docs/src/integrate/integration-tutorial.md Outdated Show resolved Hide resolved
@github-actions
Copy link

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.

@github-actions github-actions bot added the Stale label May 15, 2023
@bpmutter bpmutter marked this pull request as ready for review May 16, 2023 01:51
@bpmutter bpmutter requested a review from a team as a code owner May 16, 2023 01:51
@bpmutter bpmutter requested a review from nzakas May 16, 2023 01:51
@github-actions github-actions bot removed the Stale label May 16, 2023
@github-actions
Copy link

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.

@github-actions github-actions bot added the Stale label May 26, 2023
@Rec0iL99 Rec0iL99 removed the Stale label May 27, 2023
@Rec0iL99
Copy link
Member

Not stale. Waiting for @nzakas's review.

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.

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.

Copy link
Member

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.

docs/src/integrate/integration-tutorial.md Outdated Show resolved Hide resolved
npm init -y
```

Install the `eslint` package as a dependency:
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

docs/src/integrate/integration-tutorial.md Outdated Show resolved Hide resolved
docs/src/integrate/integration-tutorial.md Outdated Show resolved Hide resolved
docs/src/integrate/integration-tutorial.md Outdated Show resolved Hide resolved

## 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).
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved

@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 4, 2023

CLA Missing ID CLA Not Signed

@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

@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@bpmutter bpmutter requested a review from nzakas June 4, 2023 23:43
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.

LGTM. Can you rebase on top of main to pull in our updated docs license?

@nzakas
Copy link
Member

nzakas commented Jun 7, 2023

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.

@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@bpmutter
Copy link
Contributor Author

bpmutter commented Jun 8, 2023

rebased!

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.

Looks good overall. A few suggestions.

docs/src/integrate/index.md Outdated Show resolved Hide resolved
docs/src/integrate/integration-tutorial.md Outdated Show resolved Hide resolved
docs/src/integrate/integration-tutorial.md Show resolved Hide resolved
Co-authored-by: Nitin Kumar <snitin315@gmail.com>
@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@jarias-lfx
Copy link

/easycla

@nzakas
Copy link
Member

nzakas commented Jun 12, 2023

@mdjermanovic do you want to review this before merging?

@mdjermanovic
Copy link
Member

@mdjermanovic do you want to review this before merging?

I'll take a look now.

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jun 14, 2023
docs/src/integrate/index.md Outdated Show resolved Hide resolved
"author": "",
"license": "ISC",
"devDependencies": {
"eslint": "^8.39.0",
Copy link
Member

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?

Comment on lines +179 to +180
// Apply automatic fixes and output fixed code
await ESLint.outputFixes(results);
Copy link
Member

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.

docs/src/integrate/integration-tutorial.md Outdated Show resolved Hide resolved
Comment on lines 187 to 188
if (results.length) {
console.log("Linting errors found!");
Copy link
Member

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.

Copy link
Contributor Author

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

* @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)
Copy link
Member

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.

Copy link
Contributor Author

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)
Copy link
Member

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.

Copy link
Contributor Author

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

Comment on lines 14 to 15
"eslint-config-airbnb-base": "^15.0.0",
"eslint-plugin-import": "^2.27.5"
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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?

bpmutter and others added 4 commits June 19, 2023 15:32
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@bpmutter
Copy link
Contributor Author

@mdjermanovic ready for re-review

@nzakas accidentally re-requested review. sorry about that. no action necessary right now

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 mdjermanovic merged commit e1314bf into eslint:main Jun 20, 2023
17 checks passed
json-derulo added a commit to json-derulo/angular-ecmascript-intl that referenced this pull request Jul 4, 2023
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
(#&#8203;1[eslint/eslint#17328))
(Milos Djermanovic)
-
[`4c50400`](https://togithub.com/eslint/eslint/commit/4c5040022639ae804c15b366afc6e64982bd8ae3)
feat: add `ecmaVersion: 2024`, regexp `v` flag parsing
(#&#8203;1[eslint/eslint#17324))
(Milos Djermanovic)
-
[`4d411e4`](https://togithub.com/eslint/eslint/commit/4d411e4c7063274d6d346f1b7ee46f7575d0bbd2)
feat: add ternaryOperandBinaryExpressions option to no-extra-parens rule
(#&#8203;1[eslint/eslint#17270))
(Percy Ma)
-
[`c8b1f4d`](https://togithub.com/eslint/eslint/commit/c8b1f4d61a256727755d561bf53f889b6cd712e0)
feat: Move `parserServices` to `SourceCode`
(#&#8203;1[eslint/eslint#17311))
(Milos Djermanovic)
-
[`ef6e24e`](https://togithub.com/eslint/eslint/commit/ef6e24e42670f321d996948623846d9caaedac99)
feat: treat unknown nodes as having the lowest precedence
(#&#8203;1[eslint/eslint#17302))
(Brad Zacher)
-
[`1866e1d`](https://togithub.com/eslint/eslint/commit/1866e1df6175e4ba0ae4a0d88dc3c956bb310035)
feat: allow flat config files to export a Promise
(#&#8203;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
(#&#8203;1[eslint/eslint#17320))
(Gweesin Chan)
-
[`7620b89`](https://togithub.com/eslint/eslint/commit/7620b891e81c234f30f9dbcceb64a05fd0dde65e)
fix: Remove `no-unused-labels` autofix before potential directives
(#&#8203;1[eslint/eslint#17314))
(Francesco Trotta)
-
[`391ed38`](https://togithub.com/eslint/eslint/commit/391ed38b09bd1a3abe85db65b8fcda980ab3d6f4)
fix: Remove `no-extra-semi` autofix before potential directives
(#&#8203;1[eslint/eslint#17297))
(Francesco Trotta)

#### Documentation

-
[`526e911`](https://togithub.com/eslint/eslint/commit/526e91106e6fe101578e9478a9d7f4844d4f72ac)
docs: resubmit pr 17115 doc changes
(#&#8203;1[eslint/eslint#17291))
(唯然)
-
[`e1314bf`](https://togithub.com/eslint/eslint/commit/e1314bf85a52bb0d05b1c9ca3b4c1732bae22172)
docs: Integration section and tutorial
(#&#8203;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
[@&#8203;eslint/js](https://togithub.com/eslint/js)[@&#8203;8](https://togithub.com/8).44.0
(#&#8203;1[eslint/eslint#17329))
(Milos Djermanovic)
-
[`a1cb642`](https://togithub.com/eslint/eslint/commit/a1cb6421f9d185901cd99e5f696e912226ef6632)
chore: package.json update for
[@&#8203;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
(#&#8203;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
(#&#8203;1[eslint/eslint#17323))
(Ziyad El Abid)
-
[`cf88439`](https://togithub.com/eslint/eslint/commit/cf884390ad8071d88eae05df9321100f1770363d)
chore: upgrade optionator@0.9.3
(#&#8203;1[eslint/eslint#17319))
(Milos Djermanovic)
-
[`9718a97`](https://togithub.com/eslint/eslint/commit/9718a9781d69d2c40b68c631aed97700b32c0082)
refactor: remove unnecessary code in `flat-eslint.js`
(#&#8203;1[eslint/eslint#17308))
(Milos Djermanovic)
-
[`f82e56e`](https://togithub.com/eslint/eslint/commit/f82e56e9acfb9562ece76441472d5657d7d5e296)
perf: various performance improvements
(#&#8203;1[eslint/eslint#17135))
(moonlightaria)
-
[`da81e66`](https://togithub.com/eslint/eslint/commit/da81e66e22b4f3d3fe292cf70c388753304deaad)
chore: update eslint-plugin-jsdoc to 46.2.5
(#&#8203;1[eslint/eslint#17245))
(唯然)
-
[`b991640`](https://togithub.com/eslint/eslint/commit/b991640176d5dce4750f7cc71c56cd6f284c882f)
chore: switch eslint-config-eslint to the flat format
(#&#8203;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>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 18, 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 18, 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: Create guide to integrate with ESLint Node.js API Change Request: Integrations docs section
7 participants