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 1 commit
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
24 changes: 20 additions & 4 deletions lib/eslint/eslint-helpers.js
View file
Open in desktop
Expand Up @@ -126,7 +126,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 @@ -162,16 +162,32 @@ async function findFiles({
return false;
}

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

// not sure how to handle negated patterns yet
if (filePattern.startsWith("!")) {
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 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/target-dir/passing.js
View file
Open in desktop
@@ -0,0 +1 @@
module.exports = true;
13 changes: 13 additions & 0 deletions tests/lib/eslint/flat-eslint.js
View file
Open in desktop
Expand Up @@ -784,6 +784,19 @@ describe("FlatESLint", () => {
assert.strictEqual(results[0].suppressedMessages.length, 0);
});

// https://github.com/eslint/eslint/issues/16260
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 report zero messages when given a '**' pattern with a .js and a .js2 file", async () => {
eslint = new FlatESLint({
ignore: false,
Expand Down