fix: do not ignore incorrect patterns with braces of single value #992
Conversation
Codecov Report
@@ Coverage Diff @@
## master #992 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 19 19
Lines 633 641 +8
Branches 148 146 -2
=========================================
+ Hits 633 641 +8
Continue to review full report at Codecov.
|
iiroj
force-pushed
the
braces-single-value
branch
from
711fdb2 to
80bd709
Compare
iiroj
force-pushed
the
braces-single-value
branch
from
80bd709 to
438b209
Compare
iiroj
changed the title
iiroj
changed the title
|
@okonet and @theKashey, what do you think of this approach? |
|
I love it! |
|
We don't seem to have a test for sequences like |
|
@okonet on second thought, this same thing could be done earlier in the chain, while validating configuration. Thoughts? We could even go further and actually check the globs are valid using |
|
I think it makes sense |
|
Believe it or not but I had #980 opened as the third tab in my browser for all this time. |
| ).map((file) => normalize(relative ? file : path.resolve(cwd, file))) | ||
| // Only worry about children of the CWD unless the pattern explicitly | ||
| // specifies that it concerns a parent directory. | ||
| const filteredFiles = relativeFiles.filter((file) => { |
There was a problem hiding this comment.
Not a real issue, but I have a habit to always provide a bit of advice if I can:
generateTask.js is more about "generate" and "task", and this moment is more about clearing the user input and some sort of validation.
Wondering - is it possible to move this logic into validateConfig, where it gravitates more remove connectivity with messages and logger. However, it will also create new connectivity between generateTasks and validateConfig which does not exist currently.
There was a problem hiding this comment.
Yes, I actually had the same thought in #992 (comment) after making these changes.
It makes sense to move it there, however currently validateConfig mostly just throws errors, and in this case it would simply warn. I guess that's ok though.
There was a problem hiding this comment.
This should now be moved to validateConfig
|
I improved the error messages generated from
|
|
@okonet We should probably also handle escaped braces like |
|
Yeah that seems like a bug on our end? If brackets are escaped, I'd not modify them. |
|
Let's see if the regex can be updated to detect this. |
iiroj
force-pushed
the
braces-single-value
branch
from
7658e3b to
c7eb8fc
Compare
…alue
Technically brace patterns like `*.{js}` are invalid because braces should
always contain at least one comma or a sequence,
for example `*.{js,ts}` or `file{1..10}.js`.
The `micromatch` library used to match patterns to files will silently ignore
such invalid braces and thus lead to the pattern always matching zero files.
This is a unintuitive, so lint-staged should normalize such cases
by simply removing the unnecessary braces before matching files.
iiroj
force-pushed
the
braces-single-value
branch
from
54f40ea to
e195b6a
Compare
|
@okonet since brace expansion supports nested braces, I have to make their validation recursive to handle cases like this: |
|
I think I managed to make it work as expected with nested braces as well... |
|
@okonet this could use a review |
|
🎉 This PR is included in version 11.1.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Technically brace patterns like
*.{js}are invalid because braces shouldalways contain at least one comma or a sequence,
for example
*.{js,ts}orfile{1..10}.js.The
micromatchlibrary used to match patterns to files will silently ignoresuch invalid braces and thus lead to the pattern always matching zero files.
This is a unintuitive, so lint-staged should normalize such cases
by simply removing the unnecessary braces before matching files.