-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Normalize glob patterns with trailing slashes #2788
Conversation
@novemberborn could you please review this |
@asaid-0 thanks for the PR, I may have a chance to look at this on the weekend. |
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.
@asaid-0 it's been a while since I've had to think about glob patterns and file paths. Why does this solution do the trick?
lib/glob-helpers.cjs
Outdated
function normalizePattern(pattern) { | ||
const cwd = process.cwd(); |
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.
cwd
needs to be an argument here. AVA determines the directory to which patterns are relevant and it may not be the CWD.
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.
thank you for pointing this out, I see now I think we should receive the CWD as an argument in the normalizeGlobs
here:
Line 35 in dc93f37
export function normalizeGlobs({extensions, files: filePatterns, ignoredByWatcher: ignoredByWatcherPatterns, providers}) { |
Then we pass it to normalizePatterns
call in the same function.
Also i noticed this line in cli.js
Line 396 in dc93f37
pattern: normalizePattern(path.relative(projectDir, path.resolve(process.cwd(), pattern))), |
which using nearly the same function path.relative, but path.resolve is being used in the second parameter to get the absolute path
lib/glob-helpers.cjs
Outdated
if (pattern.startsWith('!./')) { | ||
return `!${pattern.slice(3)}`; | ||
if (pattern.startsWith('!')) { | ||
return normalizeNegatedPattern(cwd, pattern); |
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.
I don't think the helper function is all that useful, especially since it hides the critical path.relative()
bit which is then called directly below.
@novemberborn Thank you for your feedback, actually, I used this solution assuming that we should also solve the issue with such patterns if we need to just solve the trailing slash issue I wouldn't use |
@asaid-0 sorry I lost track of this. These are glob patterns so I'm not sure how safe it is to pass them to |
Fixes #2781