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

Zero-padding not supported in compiled output #29

Closed
koopero opened this issue Jun 15, 2019 · 4 comments
Closed

Zero-padding not supported in compiled output #29

koopero opened this issue Jun 15, 2019 · 4 comments

Comments

@koopero
Copy link
Contributor

koopero commented Jun 15, 2019

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 pushed a commit to koopero/braces that referenced this issue Jun 15, 2019
@koopero
Copy link
Contributor Author

koopero commented Jun 15, 2019

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
Copy link
Member

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
Copy link
Contributor Author

koopero commented Jun 16, 2019

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
Copy link
Member

jonschlinkert commented May 21, 2024

This should be resolved by #29. Another change needed to be made in fill-range to ensure that padding was applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants