-
-
Notifications
You must be signed in to change notification settings - Fork 25
fix: consider negated patterns universal #220
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
Conversation
it('should return "unconfigured" when passed docx filename', () => { | ||
const filename = "sss.docx"; | ||
|
||
assert.strictEqual( | ||
configs.getConfigStatus(filename), | ||
"matched", | ||
"unconfigured", | ||
); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was added in #7, where part of the change was to update isFileIgnored()
to return true
only if the file is "really" ignored, that is, ignored by ignore patterns and not just unmatched. I think the test was added to correlate with a similar test for isFileIgnored()
, and was rather describing the current behavior at the time than insisting that the behavior is correct in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, tests for isIgnored
methods with a .docx
file, that were added at the time when unmatched files were considered the same as ignored, were asserting that this filename should not be ignored, i.e., that it should be matched, so that might have been the intent.
Here's the commit that added the original test with a .docx
file:
It's interesting that the test description says true
but then asserts that it is false
.
it('should return true when passed docx filename', () => {
const filename = path.resolve(basePath, 'sss.docx');
expect(configs.isIgnored(filename)).to.be.false;
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in that test getConfigStatus
is returning "matched"
because one of the config objects contains a negated pattern, which is being treated as a non-universal match according to the current logic:
rewrite/packages/config-array/tests/config-array.test.js
Lines 82 to 87 in e84cbd7
{ | |
files: ["!*.css"], | |
defs: { | |
css: false, | |
}, | |
}, |
This was the expected behavior prior to this fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would like @fasttime to review before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Prerequisites checklist
What is the purpose of this pull request?
Fixes the problem described in eslint/eslint#19813.
What changes did you make? (Give an overview)
Updated regex pattern that checks for universal patterns to treat negated patterns as such.
Related Issues
eslint/eslint#19813
Is there anything you'd like reviewers to focus on?