fix: npm ignore file was not getting picked #213
Conversation
| @@ -123,7 +127,7 @@ class PackWalker extends IgnoreWalker { | |||
| // then we know path is a workspace directory. in order to not drop ignore rules | |||
| // from directories between the workspaces root (prefix) and the workspace itself | |||
| // (path) we need to find and read those now | |||
| const relpath = relative(options.prefix, dirname(options.path)) | |||
There was a problem hiding this comment.
Why is dirname being removed here?
There was a problem hiding this comment.
dirname was removing last directory and resulting into one directory behind..
if path = root/level1/level2
dirname(path) = root/level1 due to this ignore files present in level2 is never got considered.
to consider level2 files, this is removed, although i had tested by keeping files at each level but let me know if i am missing anything
There was a problem hiding this comment.
Given the state of the existing tests in this I'm very hesitant to remove something like this without fully understanding the side effects. We've had several bugs here in the last few months that weren't caught due to tests. If this is fixing a bug, we'd want to find out what that bug is and clearly define it in a test. If it's introducing a bug we are obviously not currently capturing it in a test and would want to make sure we add one to prevent this kind of regressions.
There was a problem hiding this comment.
Definitely we need to add more test case to cover both regression and the bug.. the current bug is captured in the test written. I will analyze more to understand why dirname was included at first place and if some test is missing to cover that.
There was a problem hiding this comment.
I don't see test cases covering ignore files at 3 level deep, its included now.
There was a problem hiding this comment.
The reason we're using dirname is because this code path is in a workspace. The comment above this line explains it. Removing dirname means we're no longer addressing the situation described in the comment.
There was a problem hiding this comment.
what i understood from comment is we want to read directories present in between (a)PREFIX and (a/b/c)WORKSPACE.. but in that case the ignore file present in the workspace(a/b/c) is never considered..
if we pass workspace(a/b/c) without dirname, recursion in "readOutOfTreeIgnoreFiles" is doing job to read files present in each subdirectories(a/b/c, a/b and a) starting from workspace directory.
|
The added test already passes without the change in |
The ignore files present into 3rd level subdirectories a\b\c\npmignore was not getting considered. dirname() was issue there which is removed now.
When its starts scanning into subdirectories we need to do recursive call to cover all npmignore and gitignore files and store it into 2 different results.
since npmignore is preferred over gitignore, it will be returned if its present in workspace.
References