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

feat!: add method getConfigStatus, update isFileIgnored #7

Merged
merged 6 commits into from
May 29, 2024

Conversation

fasttime
Copy link
Member

@fasttime fasttime commented May 7, 2024

This PR adds a method getConfigStatus to ConfigArray to determine the reason a file has no matching config. This could be used in ESLint to provide a better message for files that don't have a matching configuration. The reason could be:

  • The file is ignored by global ignores patterns
  • The file is outside the base path
  • The file has no non-universal files pattern matching

There is also a new method getConfigWithStatus that returns a config object and the status for a file. This is now the backing implementation for both getConfig and getConfigStatus.

This PR also changes the behavior of the methods isFileIgnored/isIgnored. These methods will now only return true for files that are ignored due to ignores patterns. This is a breaking change.

Refs eslint/eslint#18263

See also the discussion in humanwhocodes/config-array#138.

@fasttime fasttime marked this pull request as ready for review May 7, 2024 07:45
@nzakas
Copy link
Member

nzakas commented May 9, 2024

I think we should revisit the expected behavior of isFileIgnored() and isExplicitMatch() before we add another method. Specifically:

  1. isFileIgnored() - this should probably return true only if a file matches an ignores pattern somewhere along the way, as opposed to right now, where it also returns true if there isn't a files pattern that matches.
  2. isExplicitMatch() - this should probably return true only if there's matching files pattern somewhere such that a config will be generated. Maybe rename to isFileMatched()?

So then we could do:

const isFileConfigured = !isFileIgnored && !isExplicitMatch;

@fasttime
Copy link
Member Author

I think we should revisit the expected behavior of isFileIgnored() and isExplicitMatch() before we add another method. Specifically:

1. `isFileIgnored()` - this should probably return `true` only if a file matches an `ignores` pattern somewhere along the way, as opposed to right now, where it also returns `true` if there isn't a `files` pattern that matches.

2. `isExplicitMatch()` - this should probably return `true` only if there's matching `files` pattern somewhere such that a config will be generated. Maybe rename to `isFileMatched()`?

So then we could do:

const isFileConfigured = !isFileIgnored && !isExplicitMatch;

Sounds good 👍🏻I can change the methods and update ESLint to see how that works in practice. This will be a breaking change.

@nzakas
Copy link
Member

nzakas commented May 13, 2024

@mdjermanovic what do you think of #7 (comment)?

@mdjermanovic
Copy link
Member

mdjermanovic commented May 14, 2024

The current isExplicitMatch() differs from other methods in that it can return true in some cases where the config won't be generated (i.e., the file is essentially ignored):

humanwhocodes/config-array#138 (comment).

In ESLint, we're using isExplicitMatch() only for filtering code blocks:

https://github.com/eslint/eslint/blob/d23574c5c0275c8b3714a7a6d3e8bf2108af60f1/lib/eslint/eslint.js#L510-L512

Now, before changing the behavior of isExplicitMatch(), I think we should first figure out whether the mentioned current specifics of it actually make sense for this use case. In other words, is the current behavior where a code block matched only by patterns that end with /* or /** ends up producing this lint error intentional and desirable, or should such code block just be silently skipped.

Edit: I tried making an example where the current behavior would be easily observable with eslint-plugin-markdown, but the lint error we're generating has line: 0 (which seems wrong because lines are 1-based) and then eslint-plugin-markdown's postprocess filters it out as out-of-range.

@nzakas
Copy link
Member

nzakas commented May 14, 2024

Now it's all coming back to me. :) So isExplicitMatch() is important to work as-is in order to allow us to filter out code blocks, otherwise all of the code blocks would always match any pattern ending with * due to its parent filename.

So then we are really talking about modifying isFileIgnored() so that it returns true only for files that explicitly ignored via an ignores entry, whereas isFileConfigured() returns true if getConfig() would return a config object?

@fasttime
Copy link
Member Author

On closer look at the implementation, I think that getConfig() could return null for any of three unrelated reasons:

  1. The file is ignored by a global ignore pattern
  2. The file has no matching config
  3. The file is outside the base path

or some combination of the above. Currently, isFileIgnored will return true for any of the above reasons . My original idea for adding isFileConfigured is that is would return true only if both 1. and 2. are false, i.e. if a file is not globally ignored and has a matching config. I thought this would be sufficient for providing a meaningful error message in ESLint, but, as it seems, I didn't consider reason 3.

@mdjermanovic
Copy link
Member

On closer look at the implementation, I think that getConfig() could return null for any of three unrelated reasons:

  1. The file is ignored by a global ignore pattern
  2. The file has no matching config
  3. The file is outside the base path

or some combination of the above. Currently, isFileIgnored will return true for any of the above reasons . My original idea for adding isFileConfigured is that is would return true only if both 1. and 2. are false, i.e. if a file is not globally ignored and has a matching config. I thought this would be sufficient for providing a meaningful error message in ESLint, but, as it seems, I didn't consider reason 3.

Yeah, while it can be argued that 3. is a subset of 2. because all patterns are considered relative to the base path so no pattern can match a file outside of it, it would still be good to show a distinct message for 3.

@mdjermanovic
Copy link
Member

Now it's all coming back to me. :) So isExplicitMatch() is important to work as-is in order to allow us to filter out code blocks, otherwise all of the code blocks would always match any pattern ending with * due to its parent filename.

Can you provide an example to clarify why the specific logic of isExplicitMatch() is needed?

I tried replacing isExplicitMatch() with getConfig() here:

https://github.com/eslint/eslint/blob/b67eba4514026ef7e489798fd883beb678817a46/lib/eslint/eslint.js#L510-L512

filterCodeBlock(blockFilename) {
-    return configs.isExplicitMatch(blockFilename);
+    return configs.getConfig(blockFilename) !== void 0;
}

and all tests we have in eslint/eslint are still passing.

@nzakas
Copy link
Member

nzakas commented May 15, 2024

I believe the case was when we had a files pattern such as "foo/**". If we had a file named foo/bar.md that then produced foo/bar.md/0.js, then the files pattern still matched against the block.

@mdjermanovic
Copy link
Member

What is the expected behavior in the following case:

// eslint.config.js
export default [
    {
        plugins: {
            "my-plugin": {
                processors: {
                    "my-processor": {

                        // dummy processor that assumes that the whole content of a file is a single ```ts code block 
                        preprocess(text) {
                            const lines = text.split("\n");
                            return [{
                                filename: "foo.ts",
                                text: lines.slice(1, -1).join("\n")
                            }]
                        },

                        // just increment lines to adjust for the first ```ts line
                        postprocess(messages) {
                            const retv = messages[0]; // there's always exactly 1 code block

                            retv.forEach(message => {
                                message.line++
                                message.endLine++; 
                            });

                            return retv;
                        }
                    }
                }
            }
        }
    },
    {
        files: ["**/*.md"],
        processor: "my-plugin/my-processor"
    },
    {
        files: ["**/*"],
        rules: {
            "no-undef": 2
        }
    }
];

a.md:

```ts
bar
```

The virtual .ts file is matched only by **/*. Should the code block be silently skipped or reported as not-configured?

The current behavior is:

C:\projects\tmp\tmp\a.md
  1:0  warning  No matching configuration found for C:\projects\tmp\tmp\a.md\0_foo.ts

✖ 1 problem (0 errors, 1 warning)

@nzakas
Copy link
Member

nzakas commented May 17, 2024

The virtual .ts file is matched only by **/*. Should the code block be silently skipped or reported as not-configured?

Yes, so this is exactly the case I was targeting. IMHO, we should silently skip code blocks that don't have an explicit match. It's quite possible that people won't want to lint every code block, especially if it's not JavaScript. Outputting a warning for every code block that doesn't have a matching config seems likely to only create noise and frustration. Think of a Markdown file that has code examples in multiple languages, most of which ESLint does not lint.

@mdjermanovic
Copy link
Member

mdjermanovic commented May 17, 2024

The virtual .ts file is matched only by **/*. Should the code block be silently skipped or reported as not-configured?

Yes, so this is exactly the case I was targeting. IMHO, we should silently skip code blocks that don't have an explicit match. It's quite possible that people won't want to lint every code block, especially if it's not JavaScript. Outputting a warning for every code block that doesn't have a matching config seems likely to only create noise and frustration. Think of a Markdown file that has code examples in multiple languages, most of which ESLint does not lint.

I agree, but isExplicitMatch() doesn't work that way. In the above example, it returns true for the virtual .ts file because it matches **/*, and at the end ESLint outputs No matching configuration found for C:\projects\tmp\tmp\a.md\0_foo.ts warning.

@nzakas
Copy link
Member

nzakas commented May 20, 2024

Okay, I got confused.

I agree, but isExplicitMatch() doesn't work that way. In the above example, it returns true for the virtual .ts file because it matches **/*, and at the end ESLint outputs No matching configuration found for C:\projects\tmp\tmp\a.md\0_foo.ts warning.

Right, that's what we don't want. So maybe isExplicitMatch() needs to change to not match **/*?

@fasttime
Copy link
Member Author

It looks like the wrong behavior of isExplicitMatch is a different problem than what this issue was supposed to address, so what do you think if we open a separate issue to discuss about isExplicitMatch and try to focus here on the different reasons why a file could end up not having a config?

@mdjermanovic
Copy link
Member

I started the discussion about isExplicitMatch because one of the proposed solutions was to modify its functionality, so it seemed important to check what we're using it for and whether we need the current functionality (so far, it seems we don't). I'll open an issue about #7 (comment) in the eslint/eslint repo.

@fasttime
Copy link
Member Author

I think that changing the return type of getConfig to include the reason or reasons why a file has no config should work. This was first suggested by @mdjermanovic in this comment. Alternatively, we could leave the functionality of getConfig unchanged and add a new method that returns the reason. The new method could replace both isFileIgnored and isFileConfigured. Thoughts?

@mdjermanovic
Copy link
Member

I think that changing the return type of getConfig to include the reason or reasons why a file has no config should work. This was first suggested by @mdjermanovic in this comment. Alternatively, we could leave the functionality of getConfig unchanged and add a new method that returns the reason. The new method could replace both isFileIgnored and isFileConfigured. Thoughts?

Sounds good to me. @nzakas what do you think?

@nzakas
Copy link
Member

nzakas commented May 24, 2024

I'm not in favor of changing getConfig() -- I think the expectation is that it either returns a config or doesn't. I also think, outside of our own use case, the reason why undefined was returned doesn't matter.

I am in favor of adding another method that can return the reason a file doesn't have a config.

@fasttime
Copy link
Member Author

I'm not in favor of changing getConfig() -- I think the expectation is that it either returns a config or doesn't. I also think, outside of our own use case, the reason why undefined was returned doesn't matter.

I am in favor of adding another method that can return the reason a file doesn't have a config.

Thanks! I will add a new method that returns the reason a file doesn't have a config and I will change the behavior of isFileIgnored() as suggested in #7 (comment). I will leave alone isExplicitMatch() in this PR since we have issue eslint/eslint#18493 to address that.

@fasttime fasttime marked this pull request as draft May 25, 2024 11:41
@fasttime fasttime changed the title feat: add method isFileConfigured feat!: add method getConfigStatus, update isFileIgnored May 26, 2024
The methods`isFileIgnored`/`isIgnored` will only return `true` if a file is globally ignored.
packages/config-array/tests/config-array.test.js Outdated Show resolved Hide resolved
packages/config-array/tests/config-array.test.js Outdated Show resolved Hide resolved
packages/config-array/tests/config-array.test.js Outdated Show resolved Hide resolved
@@ -1005,50 +1792,6 @@ describe("ConfigArray", () => {

assert.strictEqual(configs.isIgnored(filename), true);
});
it("should return true when passed matching both files and ignores in a config", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Many of the tests without global "ignores" patterns are no longer relevant to isIgnored()/isFileIgnored() now, so I removed them. The removed test cases are all covered by the tests for getConfigStatus().

@fasttime
Copy link
Member Author

fasttime commented May 26, 2024

This is ready for re-review. I stumbled on three unit tests that don't quite understand, so please have look at the review comments.

@fasttime fasttime marked this pull request as ready for review May 26, 2024 10:15
Comment on lines 1090 to 1092
isFileIgnored(filePath) {
return this.getConfig(filePath) === undefined;
return this.getConfigStatus(filePath) === "ignored";
}
Copy link
Member

Choose a reason for hiding this comment

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

In ESLint, we'll need to update all uses of isFileIgnored() to getConfig() or getConfigStatus() in order to retain current functionalities, which I think raises the question of whether isFileIgnored is needed anymore. I think we should just remove it.

Copy link
Member

Choose a reason for hiding this comment

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

I still like it as a shorthand for getConfigStatus() === "ignored".

* @returns {"ignored"|"external"|"unconfigured"|"matched"} One of the constants returned by
* {@linkcode ConfigArray.getConfigStatus}.
*/
function calculateConfigStatus(configArray, filePath) {
Copy link
Member

Choose a reason for hiding this comment

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

There's a lot of repeated logic from getConfig, and a separate cache.

Maybe we could rename the current getConfig to getConfigWithStatus, and modify it to cache and return { config, status }? Then, the new getConfig would be just return this.getConfigWithStatus(filePath).config;, and getConfigStatus would be just return this.getConfigWithStatus(filePath).status;.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I also thought that. Would like @nzakas' perspective on this.

Copy link
Member

Choose a reason for hiding this comment

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

I think that makes sense. 👍

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 suggestion, done in 5ce6b18. Please, have a look.

@@ -2334,14 +2922,11 @@ describe("ConfigArray", () => {
);
});

it("should return false when first-level subdirectories are ignored with leading slash and then one is negated", () => {
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 test was bogus because the first ignore pattern was not ignoring the subdirectory path, so I removed the negated pattern and changed the description to match what is going on.

cache.set(filePath, finalConfig);
return finalConfig;
// cache and return result
configWithStatus = Object.freeze({ status: "unconfigured" });
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts about preparing these objects in advance, so that we don't create a new instance for each unconfigured (& ignored & external) file?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in b194ef8, thanks!

@nzakas
Copy link
Member

nzakas commented May 28, 2024

Before we merge, we should also check how the new behavior of isFileIgnored() affects the directory traversal in ESLint.

@mdjermanovic
Copy link
Member

Before we merge, we should also check how the new behavior of isFileIgnored() affects the directory traversal in ESLint.

It breaks it, we'll need to update all calls to .isFileIgnored() to something like .getConfig() === void 0 when we switch to @eslint/config-array.

@fasttime
Copy link
Member Author

fasttime commented May 29, 2024

I tried to run the tests in ESLint with a version of config-array built from this PR, and there were failures as expected. But after replacing the occurrences of isFileIgnored() with !getConfig() (there are three), the tests were passing again. In one case, the check with isFileIgnored seems untested (or unnecessary): this line could be replaced with just return matchesPattern; and all tests will still pass.

So it will be a breaking upgrade when we switch to @eslint/config-array in ESLint, but so far the fix should be easy.

Copy link
Member

@mdjermanovic mdjermanovic 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! Leaving open for @nzakas to verify.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM.

Because this is a new package name, I'm not too concerned with this being a breaking change as older ESLint's won't automatically get this update.

@nzakas nzakas merged commit 400c5f9 into main May 29, 2024
14 checks passed
@nzakas nzakas deleted the eslint-issue-18263 branch May 29, 2024 13:44
@github-actions github-actions bot mentioned this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants