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

Update: lint code block with same extension but different content #14227

Merged
merged 2 commits into from Apr 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
6 changes: 3 additions & 3 deletions lib/linter/linter.js
Expand Up @@ -1308,9 +1308,9 @@ class Linter {
return [];
}

// Resolve configuration again if the file extension was changed.
if (configForRecursive && path.extname(blockName) !== originalExtname) {
debug("Resolving configuration again because the file extension was changed.");
// Resolve configuration again if the file content or extension was changed.
if (configForRecursive && (text !== blockText || path.extname(blockName) !== originalExtname)) {
debug("Resolving configuration again because the file content or extension was changed.");
Comment on lines -1311 to +1313
Copy link
Member

Choose a reason for hiding this comment

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

This is potentially a breaking change. The current behavior when preprocess() returns the same extension isn't well documented, but maybe some processors don't expect that they'll be run again on a part of content that originates from the same file. This also changes how existing user configurations will be interpreted (#14207).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any example that this change will break some processors? I can't imagine that.

And It's just the correct behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And by referencing my original issue, I don't understand what will change, I tried this change locally, rules of *.foo/*(including *.foo/*.foo) take higher priority than *.foo, it is very expected to me.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with you that the current behavior looks unexpected, but we can't really know all third-party processors to tell if this will break something. I marked this PR as "breaking" until we figure out whether the original issue is a bug or just an undocumented exception in the behavior, and estimate the impact of a fix if it's a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then how do you figure out whether it's a bug?

Copy link
Member

Choose a reason for hiding this comment

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

I see part 2 as a bug fix that will break at least one processor and possibly others.

Is it still an open question whether we will fix only 1, or both 1 and 2?

Copy link
Contributor Author

@JounQin JounQin Apr 9, 2021

Choose a reason for hiding this comment

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

As @btmills mentioned, foo.md/0_0.markdown/0_0.js is already been linted now, so 2) should be fixed too.

Besides, this issue has been fixed by eslint/eslint-plugin-markdown#183.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I noticed that eslint-plugin-markdown has been fixed and released, I'm just concerned about the "possibly others" part.

Copy link
Contributor Author

@JounQin JounQin Apr 9, 2021

Choose a reason for hiding this comment

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

That issue is not caused by this fix at all, and also md -> html -> js, so I don't think we should concern about that part.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. If other processors have the same implementation bug as eslint-plug-in-markdown, it's already possible to repro with a different syntax in between, so this is making it slightly easier to encounter in the worst case.

return this._verifyWithConfigArray(
blockText,
configForRecursive,
Expand Down
48 changes: 48 additions & 0 deletions tests/lib/eslint/eslint.js
Expand Up @@ -2917,6 +2917,54 @@ describe("ESLint", () => {
assert.strictEqual(results[0].messages[0].ruleId, "post-processed");
});

it("should run processors when calling lintText with processor resolves same extension but different content correctly", async () => {
let count = 0;

eslint = new ESLint({
useEslintrc: false,
overrideConfig: {
plugins: ["test-processor"],
overrides: [{
files: ["**/*.txt/*.txt"],
rules: {
"no-console": 2,
"no-unused-vars": 2
}
}]
},
extensions: ["txt"],
ignore: false,
plugins: {
"test-processor": {
processors: {
".txt": {
preprocess(text) {
count++;
return [
{

// it will be run twice, and text will be as-is at the second time, then it will not run third time
text: text.replace("a()", "b()"),
filename: ".txt"
}
];
},
postprocess(messages) {
messages[0][0].ruleId = "post-processed";
return messages[0];
}
}
}
}
}
});
const results = await eslint.lintText("function a() {console.log(\"Test\");}", { filePath: "tests/fixtures/processors/test/test-processor.txt" });

assert.strictEqual(count, 2);
assert.strictEqual(results[0].messages[0].message, "'b' is defined but never used.");
assert.strictEqual(results[0].messages[0].ruleId, "post-processed");
});

describe("autofixing with processors", () => {
const HTML_PROCESSOR = Object.freeze({
preprocess(text) {
Expand Down