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

Regression since 2.1.1 #59

Open
novemberborn opened this issue Jan 5, 2020 · 6 comments
Open

Regression since 2.1.1 #59

novemberborn opened this issue Jan 5, 2020 · 6 comments

Comments

@novemberborn
Copy link

👋 thanks for your hard work on this package.

I have patterns like this:

'**/__tests__/**/__{helper|fixture}?(s)__/**/*',
'**/test?(s)/**/{helper|fixture}?(s)/**/*'

With the latest release these no longer match.

I'm trying to wrap up a release so don't have the time to make an isolated reproduction, but if you check out this PR and revert the last commit, and then run npx tap --no-cov test/globs.js you should see the test failures.

Let me know if you'd like more details and I'll try and find the time to help out. Thanks!

@jonschlinkert
Copy link
Member

Very sorry for the regression and thanks for creating the issue!

check out this PR and revert the last commit, and then run npx tap --no-cov test/globs.js

Can you provide a reduced test case please? I don't use the same tooling, and I have limited time for the next couple of days. If you can show me exactly what's failing I can probably push up a patch quickly.

@novemberborn
Copy link
Author

I have limited time for the next couple of days.

I'm in the same boat unfortunately. Will respond back when I can.

@jonschlinkert
Copy link
Member

@novemberborn I completely missed the part of your comment where you said "don't have the time to make an isolated reproduction". Sorry about that, I think I read the comment three times before I noticed that part for some reason. I think my eyes are trained (in a bad way) to look for code lol.

I'm looking into it, thanks.

@jonschlinkert
Copy link
Member

@novemberborn ok I figured out the issue. In brace patterns you are using pipes instead of commas to separate patterns.

https://github.com/avajs/ava/blob/9ea96086f85d9cb10e3b1a8cf96047615c41b855/lib/globs.js#L56-L67

For now, you can fix those tests by simply changing | to , in the brace patterns. Although this is correct, it worked before my recent patches, so It seems that I inadvertently introduced a regression.

I will also push up a patch to allow the old behavior to work... specifically, braces with a single pattern are regarded as literals, and since {a|b} is technically a single pattern, it's converted to a literal.

@novemberborn
Copy link
Author

@jonschlinkert thanks for looking into this!

Yes , makes more sense here. Not sure why I wrote it with |.

I will also push up a patch to allow the old behavior to work... specifically, braces with a single pattern are regarded as literals, and since {a|b} is technically a single pattern, it's converted to a literal.

Should that old behavior have worked at all?

@jonschlinkert
Copy link
Member

Should that old behavior have worked at all?

No, probably not, but since it did it's safer to keep support for that behavior until we publish the next major release.

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