micromatch / braces

Faster brace expansion for node.js. Besides being faster, braces is not subject to DoS attacks like minimatch, is more accurate, and has more complete support for Bash 4.3.
https://github.com/jonschlinkert
MIT License
207 stars 47 forks source link

Zero-padding not supported in compiled output #29

Closed koopero closed 1 month ago

koopero commented 5 years ago

It appears that zero-padded numeric sequences are advertised, but not properly supported.

// Example from README.md
console.log(braces('a/{001..300}/b')); //=> ['a/(0{2}[1-9]|0[1-9][0-9]|[12][0-9]{2}|300)/b']

// Actual output is:
[ 'a/(0{0,2}[1-9]|0?[1-9][0-9]|[12][0-9]{2}|300)/b' ]

Will follow up with PR with failing test cases.

Please advice whether the fix for this is in this module, or upstream in to-regex-range.

As always, thanks for you hard work on this module and so many others.

koopero commented 5 years ago

Okay, I dug into this a bit more...

It seems that fill-range is the culprit, and 'to-regex-range' is not actually used. The undocumented strictZeros option can be passed to fix zero-padding in braces.

assert.deepEqual( braces(['a/{01..20}/b', 'a/{1..5}/b'], { strictZeros: true } ), [ 'a/(0[1-9]|1[0-9]|20)/b', 'a/([1-5])/b' ] );

My recommended solution here is to make { strictZeros: true } the default behavior in braces, since ( to my mind anyway ) it represents a more correct output, leaves fill-range intact and still allows overriding to non-strict zeros.

Thoughts?

jonschlinkert commented 5 years ago

The undocumented strictZeros option can be passed to fix zero-padding in braces.

Thank you, I had a hard time remembering and was about to look. Yes, I agree with that. However, I think we should do a major bump because it will result in a breaking change. Would you like to do a PR?

FWIW I have a couple of other fixes that I'll be publishing as a patch before I bump the major.

koopero commented 5 years ago

I will make a PR for this in the next few days, making { strictZeros: true } the default behavior and adding docs and test cases. Is there any reason to use to-regex-range, or is fill-range sufficient?

jonschlinkert commented 1 month ago

This should be resolved by https://github.com/micromatch/braces/issues/29. Another change needed to be made in fill-range to ensure that padding was applied.