fix(deps): pathspec update #25
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #25 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 196 196
Branches 34 34
=========================================
Hits 196 196 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @frichtarik 🙌
Yes, I agree! Limiting pathspec, as per your PR, was a good temporary solution to decouple other critical fixes from changing the behavior.
Regarding:
it expected to see visible files inside invisible folder, which is not a real situation
I think this could be something you might want to do (as in "be able to express") but the main point is what is expected for the specific match patterns provided.
In fact, there is no comprehensive definition for the patterns in the dirhash-standard (which is a critical weakness currently :/) but I think we should go by the claim in the README:
Glob/wildcard (".gitignore style") path matching for expressive filtering of files to include/exclude.
So we should follow how git/.gitignore works (which is exactly what pathspec fixed from >0.10 as I understand it). For future reference, a shell version of the updated test:
mkdir temp
cd temp
git init
mkdir d1 .d2
touch f1 .f2
touch d1/f1 d1/.f2
touch .d2/f1
echo ".*\n" > .gitignore
git add .
git statusOn branch master
No commits yet
Changes to be committed:
(use "git rm --cached <file>..." to unstage)
new file: d1/f1
new file: f1
rm .gitignore
git statusOn branch master
No commits yet
Changes to be committed:
(use "git rm --cached <file>..." to unstage)
new file: d1/f1
new file: f1
Untracked files:
(use "git add <file>..." to include in what will be committed)
.d2/
.f2
d1/.f2
Please just see one comment re. the updated test case 🙏
tests/test_dirhash.py
Outdated
| @@ -284,9 +284,9 @@ def test_ignore_hidden_files(self): | |||
| # with ignore | |||
| filepaths = included_paths( | |||
| self.path_to('root'), | |||
| match=['*', '!.*'] | |||
| match=['*', '!**/.*'] | |||
There was a problem hiding this comment.
Could I please ask you to revert this and keep the original pattern here - this way it is clear that the expected behavior for the same scenario (same directory content and patterns) has changed as per this PR 👍
When we change both the patterns and the result one might think that the new outcome is attributed to the changed pattern, if you see my point?
So maybe rename this test case to just test_ignore_hidden (since it is not only ignoring .files) and duplicate it to a new test case named test_ignore_hidden_explicitly_recursive with your new pattern (I mean the plain .* also, in fact, matches recursively since d1/.f2 is ignored)
There was a problem hiding this comment.
Hi @andhus, i understand your concerns, thanks for the review.
In the end i've split the original test into 3, i'll add comments to the diff for simpler re-review
|
And thanks for bumping the github action versions 👌 |
| @@ -267,7 +267,7 @@ def test_cyclic_link(self): | |||
| with pytest.raises(SymlinkRecursionError): | |||
| filepaths = included_paths(self.path_to('root')) | |||
|
|
|||
| def test_ignore_hidden_files(self): | |||
| def test_ignore_hidden(self): | |||
There was a problem hiding this comment.
- test (original) - i changed the name and the expected result for ignore codeblock, skipping '.d2/f1'
| filepaths = included_paths(self.path_to('root'), match=['*', '!.*']) | ||
| assert filepaths == ['d1/f1', 'f1'] | ||
|
|
||
| def test_ignore_hidden_files_only(self): |
There was a problem hiding this comment.
- test - after reading through suggested glob documentation, i was able to figure out combination of patterns that allow the original ignore result including visible file inside of hidden folder '.d2/f1'
| assert filepaths == ['.d2/f1', 'd1/f1', 'f1'] | ||
|
|
||
| def test_ignore_hidden_explicitly_recursive(self): |
There was a problem hiding this comment.
- test - pattern
'!**/.*'that leads to the same result as'!.*'(test 1) with the latest pathspec
removing limit to pathspec<0.10.0 as the one failing integration test was IMHO incorrect
it expected to see visible files inside invisible folder, which is not a real situation
pathspec did fix the behavior, and so should we