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.

Sign up for GitHub

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

Jump to bottom

fix: Ensure globbing doesn't include subdirectories #16272

Merged
merged 5 commits into from Sep 12, 2022
Merged

fix: Ensure globbing doesn't include subdirectories #16272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
46017f1
fix: Ensure globbing doesn't include subdirectories
nzakas Sep 1, 2022
315dd71
docs: copy & use main package version in docs on release (#16252)
jugalthakkar Sep 5, 2022
dd9612c
chore: enable linting `.eleventy.js` again (#16274)
mdjermanovic Sep 5, 2022
1f9ccdc
Fix filtering of globs
nzakas Sep 5, 2022
8fc387c
Enable partial matching of globs
nzakas Sep 7, 2022
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
9 changes: 8 additions & 1 deletion Makefile.js
View file
Open in desktop
Expand Up @@ -287,6 +287,13 @@ function generateRelease() {
generateBlogPost(releaseInfo);
commitSiteToGit(`v${releaseInfo.version}`);

echo("Updating version in docs package");
const docsPackagePath = path.join(__dirname, "docs", "package.json");
const docsPackage = require(docsPackagePath);

docsPackage.version = releaseInfo.version;
fs.writeFileSync(docsPackagePath, `${JSON.stringify(docsPackage, null, 4)}\n`);

echo("Updating commit with docs data");
exec("git add docs/ && git commit --amend --no-edit");
exec(`git tag -a -f v${releaseInfo.version} -m ${releaseInfo.version}`);
Expand Down Expand Up @@ -538,7 +545,7 @@ target.lintDocsJS = function([fix = false] = []) {
let errors = 0;

echo("Validating JavaScript files in the docs directory");
const lastReturn = exec(`${ESLINT}${fix ? "--fix" : ""} docs`);
const lastReturn = exec(`${ESLINT}${fix ? "--fix" : ""} docs/.eleventy.js`);

if (lastReturn.code !== 0) {
errors++;
Expand Down
2 changes: 1 addition & 1 deletion docs/package.json
View file
Open in desktop
@@ -1,7 +1,7 @@
{
"name": "docs-eslint",
"private": true,
"version": "1.0.0",
"version": "8.23.0",
"description": "",
"main": "index.js",
"keywords": [],
Expand Down
4 changes: 2 additions & 2 deletions docs/src/_data/eslintVersion.js
View file
Open in desktop
Expand Up @@ -14,7 +14,7 @@ const path = require("path");
// Initialization
//-----------------------------------------------------------------------------

const pkgPath = path.resolve(__dirname, "../../../package.json");
const pkgPath = path.resolve(__dirname, "../../package.json");
const pkg = JSON.parse(fs.readFileSync(pkgPath, "utf8"));
const { ESLINT_VERSION } = process.env;

Expand All @@ -23,7 +23,7 @@ const { ESLINT_VERSION } = process.env;
//-----------------------------------------------------------------------------

/*
* Because we want to differentiate between the development branch and the
* Because we want to differentiate between the development branch and the
* most recent release, we need a way to override the version. The
* ESLINT_VERSION environment variable allows us to set this to override
* the value displayed on the website. The most common case is we will set
Expand Down
19 changes: 3 additions & 16 deletions eslint.config.js
View file
Open in desktop
Expand Up @@ -77,10 +77,11 @@ function createInternalFilesPatterns(pattern = null) {
}

module.exports = [
...compat.extends("eslint", "plugin:eslint-plugin/recommended"),
...compat.extends("eslint"),
{
plugins: {
"internal-rules": internalPlugin
"internal-rules": internalPlugin,
"eslint-plugin": eslintPlugin
},
languageOptions: {
ecmaVersion: "latest"
Expand All @@ -96,20 +97,6 @@ module.exports = [
}
},
rules: {
"eslint-plugin/consistent-output": "error",
"eslint-plugin/no-deprecated-context-methods": "error",
"eslint-plugin/no-only-tests": "error",
"eslint-plugin/prefer-message-ids": "error",
"eslint-plugin/prefer-output-null": "error",
"eslint-plugin/prefer-placeholders": "error",
"eslint-plugin/prefer-replace-text": "error",
"eslint-plugin/report-message-format": ["error", "[^a-z].*\\.$"],
"eslint-plugin/require-meta-docs-description": "error",
"eslint-plugin/require-meta-has-suggestions": "error",
"eslint-plugin/require-meta-schema": "error",
"eslint-plugin/require-meta-type": "error",
"eslint-plugin/test-case-property-ordering": "error",
"eslint-plugin/test-case-shorthand-strings": "error",
"internal-rules/multiline-comment-style": "error"
}
},
Expand Down
30 changes: 26 additions & 4 deletions lib/eslint/eslint-helpers.js
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@ const fsp = fs.promises;
const isGlob = require("is-glob");
const globby = require("globby");
const hash = require("../cli-engine/hash");
const minimatch = require("minimatch");

//-----------------------------------------------------------------------------
// Errors
Expand Down Expand Up @@ -126,7 +127,7 @@ async function findFiles({
const filePaths = patterns.map(filePath => path.resolve(cwd, filePath));
const stats = await Promise.all(
filePaths.map(
filePath => fsp.stat(filePath).catch(() => {})
filePath => fsp.stat(filePath).catch(() => { })
)
);

Expand Down Expand Up @@ -157,6 +158,11 @@ async function findFiles({
return false;
}

// patterns starting with ** always apply
if (filePattern.startsWith("**")) {
return true;
}

// patterns ending with * are not used for file search
if (filePattern.endsWith("*")) {
return false;
Expand All @@ -167,11 +173,27 @@ async function findFiles({
return false;
}

// check if the pattern would be inside the cwd or not
// check if the pattern would be inside the config base path or not
const fullFilePattern = path.join(cwd, filePattern);
const relativeFilePattern = path.relative(configs.basePath, fullFilePattern);
const patternRelativeToConfigBasePath = path.relative(configs.basePath, fullFilePattern);

if (patternRelativeToConfigBasePath.startsWith("..")) {
return false;
}

// check if the pattern matches
if (minimatch(filePath, path.dirname(fullFilePattern), { partial: true })) {
return true;
}

// check if the pattern is inside the directory or not
const patternRelativeToFilePath = path.relative(filePath, fullFilePattern);

if (patternRelativeToFilePath.startsWith("..")) {
return false;
}
Comment on lines +189 to +194
Copy link
Member

Choose a reason for hiding this comment

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

This won't work well when ** (or anything else that isn't a literal directory name) is in the middle of filePattern.

For example:

// eslint.config.js
module.exports = {
    files: ["foo/**/*.md"],
    // ...configuration for .md files
};

Command eslint foo/bar, where foo/bar is an existing directory, would not find .md files in foo/bar directory because patternRelativeToFilePath will be calculated as ..\**\*.md.

Copy link
Member

Choose a reason for hiding this comment

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

It also doesn't work when the pattern starts with a non-literal (except for the special-cased **).

For example, if the config entry has files: ["{foo,bar}/*.md"], command eslint foo won't find .md files in foo because patternRelativeToFilePath is ..\{foo,bar}\*.md, so the pattern gets filtered out here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, yeah, this is the trickiest part of all of this. Do you have any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

For this step (filtering out patterns that can't match inside the given directory), I think we could use minimatch with partial: true. But then there's the next step where we should adjust the remaining patterns to match only inside the given directory, which seems overly complex.

Copy link
Member

Choose a reason for hiding this comment

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

Some alternative approaches:

  1. Use globby to simply get all files in the directory (if the given directory is foo/bar, pass foo/bar/**) and then filter out those that don't match at least one pattern that doesn't end with a *. I think this solution is the least error-prone.
  2. Negative patterns work as logical AND, so it might be possible to get the intersection by appending some form of double negation to the list of patterns.

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 for the suggestions. These were super helpful. I went with a minimatch-based approach that seems to work. I didn't need to use partial: true because we actually do have the full path at that point.


return !relativeFilePattern.startsWith("..");
return true;
})
.map(filePattern => {
if (filePattern.startsWith("**")) {
Expand Down
5 changes: 5 additions & 0 deletions tests/fixtures/shallow-glob/eslint.config.js
View file
Open in desktop
@@ -0,0 +1,5 @@
module.exports = [
{
files: ["subdir/*.js"]
}
];
1 change: 1 addition & 0 deletions tests/fixtures/shallow-glob/subdir/broken.js
View file
Open in desktop
@@ -0,0 +1 @@
module.exports = /* intentional syntax error */
1 change: 1 addition & 0 deletions tests/fixtures/shallow-glob/subdir/subsubdir/broken.js
View file
Open in desktop
@@ -0,0 +1 @@
function( {} // intentional syntax error
1 change: 1 addition & 0 deletions tests/fixtures/shallow-glob/subdir/subsubdir/plain.jsx
View file
Open in desktop
@@ -0,0 +1 @@
foo;
1 change: 1 addition & 0 deletions tests/fixtures/shallow-glob/target-dir/passing.js
View file
Open in desktop
@@ -0,0 +1 @@
module.exports = true;
66 changes: 66 additions & 0 deletions tests/lib/eslint/flat-eslint.js
View file
Open in desktop
Expand Up @@ -784,6 +784,72 @@ describe("FlatESLint", () => {
assert.strictEqual(results[0].suppressedMessages.length, 0);
});

// https://github.com/eslint/eslint/issues/16260
describe("Globbing based on configs", () => {
it("should report zero messages when given a directory with a .js and config file specifying a subdirectory", async () => {
eslint = new FlatESLint({
ignore: false,
cwd: getFixturePath("shallow-glob")
});
const results = await eslint.lintFiles(["target-dir"]);

assert.strictEqual(results.length, 1);
assert.strictEqual(results[0].messages.length, 0);
assert.strictEqual(results[0].suppressedMessages.length, 0);
});

it("should glob for .jsx file in a subdirectory of the passed-in directory and not glob for any other patterns", async () => {
eslint = new FlatESLint({
ignore: false,
overrideConfigFile: true,
overrideConfig: {
files: ["subdir/**/*.jsx", "target-dir/*.js"],
languageOptions: {
parserOptions: {
jsx: true
}
}
},
cwd: getFixturePath("shallow-glob")
});
const results = await eslint.lintFiles(["subdir/subsubdir"]);

assert.strictEqual(results.length, 2);
assert.strictEqual(results[0].messages.length, 1);
assert(results[0].messages[0].fatal, "Fatal error expected.");
assert.strictEqual(results[0].suppressedMessages.length, 0);
assert.strictEqual(results[1].messages.length, 0);
assert.strictEqual(results[1].suppressedMessages.length, 0);
});

it("should glob for all files in subdir when passed-in on the command line with a partial matching glob", async () => {
eslint = new FlatESLint({
ignore: false,
overrideConfigFile: true,
overrideConfig: {
files: ["s*/subsubdir/*.jsx", "target-dir/*.js"],
languageOptions: {
parserOptions: {
jsx: true
}
}
},
cwd: getFixturePath("shallow-glob")
});
const results = await eslint.lintFiles(["subdir"]);

assert.strictEqual(results.length, 3);
assert.strictEqual(results[0].messages.length, 1);
assert(results[0].messages[0].fatal, "Fatal error expected.");
assert.strictEqual(results[0].suppressedMessages.length, 0);
assert.strictEqual(results[1].messages.length, 1);
assert(results[0].messages[0].fatal, "Fatal error expected.");
assert.strictEqual(results[1].suppressedMessages.length, 0);
assert.strictEqual(results[2].messages.length, 0);
assert.strictEqual(results[2].suppressedMessages.length, 0);
});
});

it("should report zero messages when given a '**' pattern with a .js and a .js2 file", async () => {
eslint = new FlatESLint({
ignore: false,
Expand Down