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: remove /* eslint-env */ comments from rule examples #18249

Merged
merged 3 commits into from Apr 2, 2024

Conversation

mdjermanovic
Copy link
Member

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:

/* eslint-env */ have no effect in flat config mode. This PR removes those comments from rule examples.

What changes did you make? (Give an overview)

  • Added a check for /* eslint-env */ comments to tools/check-rule-examples.js
  • Removed /* eslint-env */ comments from rule examples.
    • Most of these comments were intended just to enable new ES syntax and/or new ES globals. Since the default is now "latest", I just removed them.
    • Several comments were enabling browser or node environments to enable specific globals. I replaced those comments with /* global */ comments.

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

@mdjermanovic mdjermanovic added documentation Relates to ESLint's documentation accepted There is consensus among the team that this change meets the criteria for inclusion labels Mar 30, 2024
@mdjermanovic mdjermanovic requested a review from a team as a code owner March 30, 2024 21:43
@github-actions github-actions bot removed the documentation Relates to ESLint's documentation label Mar 30, 2024
Copy link

netlify bot commented Mar 30, 2024

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 70ce4bc
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/660bb4217def4e00087a9beb

Comment on lines -90 to -101
Examples of **incorrect** code for the `{ "builtinGlobals": true }` option and the `browser` environment:

::: incorrect { "sourceType": "script" }

```js
/*eslint no-redeclare: ["error", { "builtinGlobals": true }]*/
/*eslint-env browser*/

var top = 0;
```

:::
Copy link
Member Author

Choose a reason for hiding this comment

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

This example would require specifying global variable in the config (/* global */ comment wouldn't do the work, it's a different check). Since our Playground doesn't support specifying globals, I removed this example.

Copy link
Member

Choose a reason for hiding this comment

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

Replacing /*eslint-env browser*/ with /*global top*/ does actually produce an incorrect example (repro). "builtinGlobals": true isn't even necessary. When you say it wouldn't work it's because it wouldn't report on the declaration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's a different kind of error. /*global top*/ is reported as redundant because the variable is declared in the code, whereas this example aimed to show an attempt to declare a variable that already exists.

@fasttime fasttime added the documentation Relates to ESLint's documentation label Mar 31, 2024
Comment on lines -90 to -101
Examples of **incorrect** code for the `{ "builtinGlobals": true }` option and the `browser` environment:

::: incorrect { "sourceType": "script" }

```js
/*eslint no-redeclare: ["error", { "builtinGlobals": true }]*/
/*eslint-env browser*/

var top = 0;
```

:::
Copy link
Member

Choose a reason for hiding this comment

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

Replacing /*eslint-env browser*/ with /*global top*/ does actually produce an incorrect example (repro). "builtinGlobals": true isn't even necessary. When you say it wouldn't work it's because it wouldn't report on the declaration?

tools/check-rule-examples.js Outdated Show resolved Hide resolved
Co-authored-by: Francesco Trotta <github@fasttime.org>
@github-actions github-actions bot removed the documentation Relates to ESLint's documentation label Apr 2, 2024
@eslint-github-bot eslint-github-bot bot added the documentation Relates to ESLint's documentation label Apr 2, 2024
@github-actions github-actions bot removed the documentation Relates to ESLint's documentation label Apr 2, 2024
Copy link
Member

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

@fasttime fasttime added the documentation Relates to ESLint's documentation label Apr 2, 2024
@fasttime fasttime merged commit 651ec91 into main Apr 2, 2024
19 checks passed
@fasttime fasttime deleted the docs-remove-eslint-env branch April 2, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

2 participants