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: check config comments in rule examples #17815

Merged
merged 3 commits into from Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/src/rules/capitalized-comments.md
Expand Up @@ -33,6 +33,7 @@ Examples of **correct** code for this rule:
:::correct

```js
/* eslint capitalized-comments: error */

// Capitalized comment

Expand Down
4 changes: 4 additions & 0 deletions docs/src/rules/no-buffer-constructor.md
Expand Up @@ -21,6 +21,8 @@ Examples of **incorrect** code for this rule:
::: incorrect

```js
/* eslint no-buffer-constructor: error */

new Buffer(5);
new Buffer([1, 2, 3]);

Expand All @@ -38,6 +40,8 @@ Examples of **correct** code for this rule:
::: correct

```js
/* eslint no-buffer-constructor: error */

Buffer.alloc(5);
Buffer.allocUnsafe(5);
Buffer.from([1, 2, 3]);
Expand Down
1 change: 1 addition & 0 deletions docs/src/rules/no-implicit-globals.md
Expand Up @@ -264,6 +264,7 @@ Examples of **correct** code for `/* exported variableName */` operation:
::: correct { "sourceType": "script" }

```js
/* eslint no-implicit-globals: error */
/* exported global_var */

var global_var = 42;
Expand Down
20 changes: 0 additions & 20 deletions docs/src/rules/strict.md
Copy link
Member Author

Choose a reason for hiding this comment

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

Some examples in this rule show the functioning of a removed option. These examples cannot be possibly tested in the Playground, so I removed correct/incorrect fences to prevent validation. The disadvantage of this solution is that it also removes the badges from the blocks on the website. Alternatively, we could add a new option to remove the "Open in Playground" buttons but keep the badges. Since this is the only rule I found that shows a removed option, I'm not sure if that effort would be justified.

Copy link
Member

Choose a reason for hiding this comment

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

The disadvantage of this solution is that it also removes the badges from the blocks on the website.

I think it's fine, as those examples do not represent how the current version of this rule works.

Expand Up @@ -271,19 +271,13 @@ This option ensures that all functions are executed in strict mode. A strict mod

Examples of **incorrect** code for this rule with the earlier default option which has been removed:

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

```js
// "strict": "error"

function foo() {
}
```

:::

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

```js
// "strict": "error"

Expand All @@ -294,12 +288,8 @@ function foo() {
}());
```

:::

Examples of **correct** code for this rule with the earlier default option which has been removed:

::: correct { "sourceType": "script" }

```js
// "strict": "error"

Expand All @@ -309,10 +299,6 @@ function foo() {
}
```

:::

::: correct { "sourceType": "script" }

```js
// "strict": "error"

Expand All @@ -321,10 +307,6 @@ function foo() {
}
```

:::

::: correct { "sourceType": "script" }

```js
// "strict": "error"

Expand All @@ -336,8 +318,6 @@ function foo() {
}());
```

:::

## When Not To Use It

In a codebase that has both strict and non-strict code, either turn this rule off, or [selectively disable it](../use/configure/rules#disabling-rules) where necessary. For example, functions referencing `arguments.callee` are invalid in strict mode. A [full list of strict mode differences](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode/Transitioning_to_strict_mode#Differences_from_non-strict_to_strict) is available on MDN.
14 changes: 10 additions & 4 deletions lib/source-code/source-code.js
Expand Up @@ -962,6 +962,15 @@ class SourceCode extends TokenStore {
varsCache.set("configGlobals", configGlobals);
}

static parseDirective(commentValue) {
Copy link
Member

Choose a reason for hiding this comment

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

SourceCode class is part of the public API. Perhaps we could rather add this in lib/linter/config-comment-parser.js?

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely! I'll do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, it's updated in in b14bf59. I also took the opportunity to add some unit tests and to remove a duplication in lib/linter/linter.js.

const { directivePart } = extractDirectiveComment(commentValue);
const match = directivesPattern.exec(directivePart);
const directiveText = match[1];
const directiveValue = directivePart.slice(match.index + directiveText.length);

return { directiveText, directiveValue };
}

/**
* Applies configuration found inside of the source code. This method is only
* called when ESLint is running with inline configuration allowed.
Expand All @@ -977,10 +986,7 @@ class SourceCode extends TokenStore {

this.getInlineConfigNodes().forEach(comment => {

const { directivePart } = extractDirectiveComment(comment.value);
const match = directivesPattern.exec(directivePart);
const directiveText = match[1];
const directiveValue = directivePart.slice(match.index + directiveText.length);
const { directiveText, directiveValue } = SourceCode.parseDirective(comment.value);

switch (directiveText) {
case "exported":
Expand Down
10 changes: 9 additions & 1 deletion tests/fixtures/bad-examples.md
@@ -1,5 +1,5 @@
---
title: Lorem Ipsum
title: no-restricted-syntax
---

This file contains rule example code with syntax errors.
Expand All @@ -24,3 +24,11 @@ const foo = "baz";
````

:::

:::correct

```js
/* eslint another-rule: error */
```

:::
3 changes: 2 additions & 1 deletion tests/tools/check-rule-examples.js
Expand Up @@ -61,8 +61,9 @@ describe("check-rule-examples", () => {
"\x1B[0m \x1B[2m12:1\x1B[22m \x1B[31merror\x1B[39m Syntax error: 'import' and 'export' may appear only with 'sourceType: module'\x1B[0m\n" +
"\x1B[0m \x1B[2m20:5\x1B[22m \x1B[31merror\x1B[39m Nonstandard language tag 'ts': use one of 'javascript', 'js' or 'jsx'\x1B[0m\n" +
"\x1B[0m \x1B[2m23:7\x1B[22m \x1B[31merror\x1B[39m Syntax error: Identifier 'foo' has already been declared\x1B[0m\n" +
"\x1B[0m \x1B[2m31:1\x1B[22m \x1B[31merror\x1B[39m Example code should contain a configuration comment like /* eslint no-restricted-syntax: error */\x1B[0m\n" +
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
"\x1B[0m\x1B[0m\n" +
"\x1B[0m\x1B[31m\x1B[1m✖ 4 problems (4 errors, 0 warnings)\x1B[22m\x1B[39m\x1B[0m\n" +
"\x1B[0m\x1B[31m\x1B[1m✖ 5 problems (5 errors, 0 warnings)\x1B[22m\x1B[39m\x1B[0m\n" +
"\x1B[0m\x1B[31m\x1B[1m\x1B[22m\x1B[39m\x1B[0m\n"
}
);
Expand Down
45 changes: 40 additions & 5 deletions tools/check-rule-examples.js
Expand Up @@ -7,10 +7,14 @@
const { parse } = require("espree");
const { readFile } = require("fs").promises;
const glob = require("glob");
const matter = require("gray-matter");
const markdownIt = require("markdown-it");
const markdownItContainer = require("markdown-it-container");
const { promisify } = require("util");
const meta = require("../docs/src/_data/rules_meta.json");
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
const markdownItRuleExample = require("../docs/tools/markdown-it-rule-example");
const ConfigCommentParser = require("../lib/linter/config-comment-parser");
const { SourceCode } = require("../lib/source-code");

//------------------------------------------------------------------------------
// Typedefs
Expand All @@ -26,19 +30,22 @@ const markdownItRuleExample = require("../docs/tools/markdown-it-rule-example");

const STANDARD_LANGUAGE_TAGS = new Set(["javascript", "js", "jsx"]);

const commentParser = new ConfigCommentParser();

/**
* Tries to parse a specified JavaScript code with Playground presets.
* @param {string} code The JavaScript code to parse.
* @param {ParserOptions} parserOptions Explicitly specified parser options.
* @returns {SyntaxError | null} A `SyntaxError` object if the code cannot be parsed, or `null`.
* @returns {{ ast: ASTNode } | { error: SyntaxError }} An AST with comments, or a `SyntaxError` object if the code cannot be parsed.
*/
function tryParseForPlayground(code, parserOptions) {
try {
parse(code, { ecmaVersion: "latest", ...parserOptions });
const ast = parse(code, { ecmaVersion: "latest", ...parserOptions, comment: true });

return { ast };
} catch (error) {
return error;
return { error };
}
return null;
}

/**
Expand All @@ -48,6 +55,8 @@ function tryParseForPlayground(code, parserOptions) {
*/
async function findProblems(filename) {
const text = await readFile(filename, "UTF-8");
const { title } = matter(text).data;
const isRuleRemoved = !Object.prototype.hasOwnProperty.call(meta, title);
const problems = [];
const ruleExampleOptions = markdownItRuleExample({
open({ code, parserOptions, codeBlockToken }) {
Expand All @@ -72,7 +81,33 @@ async function findProblems(filename) {
});
}

const error = tryParseForPlayground(code, parserOptions);
const { ast, error } = tryParseForPlayground(code, parserOptions);

if (ast && !isRuleRemoved) {
const hasRuleConfigComment = ast.comments.some(
comment => {
if (comment.type !== "Block" || !/^\s*eslint(?!\S)/u.test(comment.value)) {
return false;
}
const { directiveValue } = SourceCode.parseDirective(comment.value);
const parseResult = commentParser.parseJsonConfig(directiveValue, comment.loc);

return parseResult.success && Object.prototype.hasOwnProperty.call(parseResult.config, title);
}
);

if (!hasRuleConfigComment) {
const message = `Example code should contain a configuration comment like /* eslint ${title}: error */`;
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved

problems.push({
fatal: false,
severity: 2,
message,
line: codeBlockToken.map[0] + 2,
column: 1
});
}
}

if (error) {
const message = `Syntax error: ${error.message}`;
Expand Down