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
fix: do not ignore incorrect patterns with braces of single value #992
Merged
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
e093494
refactor: use arrow function
iiroj e6c62de
fix: correctly handle incorrect patterns with braces of only single v…
iiroj cf6acdc
test: add test for matching pattern with bracket sequence
iiroj 799f55a
refactor: less nesting in validateConfig
iiroj 80fb012
refactor: move pattern fixing to validateConfig
iiroj 40682e9
refactor: move error to messages
iiroj f527a25
refactor: throw early for empty configuration
iiroj dac0151
fix: improve configuration error output
iiroj 6d381a4
refactor: move formatConfig into validateConfig
iiroj 995913b
test: update snapshot serializer to remove absolute file path
iiroj a167697
fix: do not pass unused logger argument
iiroj 7dbaa29
refactor: move config validation logging to validateConfig
iiroj f538361
fix: do not match escaped curly braces (`\{` or `\}`)
iiroj 23ef46b
test: better error message in snapshot
iiroj 665ac50
test: add missing assertions
iiroj 640e58e
fix: match braces with escaped comma inside
iiroj 51a63ea
fix: do not match braces when they start with the dollar sign (`$`)
iiroj e195b6a
docs: update BRACES_REGEXP comment
iiroj 87219a5
refactor: move brace pattern validation to separate file
iiroj 089e683
fix: correctly handle nested braces
iiroj 2bd598f
refactor: clean up code
iiroj 4ae8143
test: add some more tests
iiroj c9a24ce
Merge branch 'master' into braces-single-value
iiroj File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
const { incorrectBraces } = require('./messages') | ||
|
||
/** | ||
* A correctly-formed brace expansion must contain unquoted opening and closing braces, | ||
* and at least one unquoted comma or a valid sequence expression. | ||
* Any incorrectly formed brace expansion is left unchanged. | ||
* | ||
* @see https://www.gnu.org/software/bash/manual/html_node/Brace-Expansion.html | ||
* | ||
* Lint-staged uses `micromatch` for brace expansion, and its behavior is to treat | ||
* invalid brace expansions as literal strings, which means they (typically) do not match | ||
* anything. | ||
* | ||
* This RegExp tries to match most cases of invalid brace expansions, so that they can be | ||
* detected, warned about, and re-formatted by removing the braces and thus hopefully | ||
* matching the files as intended by the user. The only real fix is to remove the incorrect | ||
* braces from user configuration, but this is left to the user (after seeing the warning). | ||
* | ||
* @example <caption>Globs with brace expansions</caption> | ||
* - *.{js,tx} // expanded as *.js, *.ts | ||
* - *.{{j,t}s,css} // expanded as *.js, *.ts, *.css | ||
* - file_{1..10}.css // expanded as file_1.css, file_2.css, …, file_10.css | ||
* | ||
* @example <caption>Globs with incorrect brace expansions</caption> | ||
* - *.{js} // should just be *.js | ||
* - *.{js,{ts}} // should just be *.{js,ts} | ||
* - *.\{js\} // escaped braces, so they're treated literally | ||
* - *.${js} // dollar-sign inhibits expansion, so treated literally | ||
* - *.{js\,ts} // the comma is escaped, so treated literally | ||
*/ | ||
const BRACES_REGEXP = /(?<![\\$])({)(?:(?!(?<!\\),|\.\.|\{|\}).)*?(?<!\\)(})/g | ||
|
||
/** | ||
* @param {string} pattern | ||
* @returns {string} | ||
*/ | ||
const withoutIncorrectBraces = (pattern) => { | ||
let output = `${pattern}` | ||
let match = null | ||
|
||
while ((match = BRACES_REGEXP.exec(pattern))) { | ||
const fullMatch = match[0] | ||
const withoutBraces = fullMatch.replace(/{/, '').replace(/}/, '') | ||
output = output.replace(fullMatch, withoutBraces) | ||
} | ||
|
||
return output | ||
} | ||
|
||
/** | ||
* Validate and remove incorrect brace expansions from glob pattern. | ||
* For example `*.{js}` is incorrect because it doesn't contain a `,` or `..`, | ||
* and will be reformatted as `*.js`. | ||
* | ||
* @param {string} pattern the glob pattern | ||
* @param {*} logger | ||
* @returns {string} | ||
*/ | ||
const validateBraces = (pattern, logger) => { | ||
const fixedPattern = withoutIncorrectBraces(pattern) | ||
|
||
if (fixedPattern !== pattern) { | ||
logger.warn(incorrectBraces(pattern, fixedPattern)) | ||
} | ||
|
||
return fixedPattern | ||
} | ||
|
||
module.exports = validateBraces | ||
|
||
module.exports.BRACES_REGEXP = BRACES_REGEXP |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 withmessages
andlogger
. However, it will also create new connectivity betweengenerateTasks
andvalidateConfig
which does not exist currently.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, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should now be moved to
validateConfig