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

The scan method returns empty parts for pattern with only a forward slash #58

Open
mrmlnc opened this issue Jan 5, 2020 · 10 comments · May be fixed by #63
Open

The scan method returns empty parts for pattern with only a forward slash #58

mrmlnc opened this issue Jan 5, 2020 · 10 comments · May be fixed by #63

Comments

@mrmlnc
Copy link
Contributor

mrmlnc commented Jan 5, 2020

Environment

  • Windows 10
  • Node.js 12.13
  • picomatch@master

Code sample

const pm = require('.');

const a = pm.scan('/', { parts: true }).parts;

console.dir(a, { colors: true });

const b = pm.isMatch('/', '/');

console.dir(b, { colors: true });

Expected behaviour

['', '']

I expect to see a representation of the pattern with each of its segments. Without empty parts of the pattern, we will not be able to apply it correctly.

For example, the current pattern must be matched only for / (left side — <empty>, right side — <empty>).

Actual behaviour

[]
@mrmlnc
Copy link
Contributor Author

mrmlnc commented Jan 5, 2020

Similar behaviour for the following patterns:

pm.scan('/a', { parts: true }).parts  []
pm.scan('/a/b', { parts: true }).parts  [ '/a', 'b' ]
pm.scan('(!(b/a))', { parts: true }).parts  []

@mrmlnc mrmlnc changed the title Scan returns empty parts for pattern with only a forward slash The scan method returns empty parts for pattern with only a forward slash Jan 5, 2020
@jonschlinkert
Copy link
Member

thanks for the issue, I'm working on it. This feedback helps.

@jonschlinkert
Copy link
Member

jonschlinkert commented Jan 6, 2020

@mrmlnc here is what I'd expect for these patterns:

pm.scan('/', { parts: true }).parts  ['', '']
pm.scan('/a', { parts: true }).parts  ['', 'a']
pm.scan('/a/b', { parts: true }).parts  ['',  'a', 'b']
pm.scan('(!(b/a))', { parts: true }).parts  ['']

I'll need to think more about the last pattern. I assume the goal is probably to create matchers for each section of a glob pattern so that you can limit fs operations, by progressively disqualifying directories. To do that properly with extglobs, we would probably need to return a proper AST so that you can create matchers around nodes in a way that just isn't possible with a flattened array of tokens.

Edit: I have a patch ready to push up once we come to consensus on what the results should be.

@mrmlnc
Copy link
Contributor Author

mrmlnc commented Jan 6, 2020

IMHO, the (!(b/a)) pattern is a difficult pattern and cannot be correctly represented by parts. But in this case, you need to explicitly indicate that this is not an array with a single item with an empty string (probably bug, an empty pattern, …), but a null or [] (empty array at all).

I correctly understand that the (a|b)/c now will be ['(a|b)', 'c']?

@jonschlinkert
Copy link
Member

I correctly understand that the (a|b)/c now will be ['(a|b)', 'c']?

yes, that's correct.

you need to explicitly indicate that this is not an array with a single item with an empty string

For the pattern (!(b/a)), IMHO the result should be ['(!(b/a))'] since the scan method returns the segments of a pattern that can be split by un-nested slashes.

@mrmlnc
Copy link
Contributor Author

mrmlnc commented Jan 9, 2020

Yeah, you're quite right there. I didn't think that only one segment of the pattern should be matched to all parts of the path.

@jonschlinkert
Copy link
Member

I think I have this working. I'm adding some unit tests to see if I can kill more edge cases before I push up. Please let me know if you come up with any patterns that you think are tricky and you want me to test. thx

@mrmlnc
Copy link
Contributor Author

mrmlnc commented Feb 29, 2020

Another case is ./directory/(a|b)/*.js:

parts: ['directory', '(a|b)/*.js' ]

I expect that (a|b) must be a separated part.

Source: mrmlnc/fast-glob#263

@jonschlinkert
Copy link
Member

Thank you for the additional test case.

However, do you mean that (a|b) should actually be split into two path segments? I don't think that's right. (a|b) is a regular expression (alternation capture group), but even if it was an extglob I don't think we'd split that pattern. IMHO, it would make more sense to do {a, b} and use brace expansion first if that is the goal.

Thoughts?

@jonschlinkert
Copy link
Member

Oh, nevermind, I completely missed the fact that there was a slash in that segment.

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

Successfully merging a pull request may close this issue.

2 participants