-
Notifications
You must be signed in to change notification settings - Fork 2.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
Fix: black only respects the root gitignore. #2225
Conversation
I noticed that we've got a failure on the test cases out of this PR scope:
For full log: check link |
That's the price we pay for using I thought it would be uncommon enough to be worth the speed improvements, but I'm start to reconsider. In the meanwhile, I'll rerun the test workflow. |
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.
This is way easier to implement than I had thought! Heh.
There's one case that this patch fails to handle correctly. It's when a parent directory doesn't have a gitignore but the child directory does. Git respects the child's gitignore but this implementation does not :(
~/programming/testing/gitignore on master via v3.8.5 (black)
❯ tree -a --matchdirs -I .git
.
├── a.py
└── some-dir
├── b.py
└── .gitignore
1 directory, 3 files
~/programming/testing/gitignore on master via v3.8.5 (black)
❯ cat some-dir/.gitignore
*.py
~/programming/testing/gitignore on master via v3.8.5 (black)
❯ git clean -xfn
Would remove some-dir/b.py # <-- git respects the child's gitignore
~/programming/testing/gitignore on master via v3.8.5 (black)
❯ black . --check -v
.git ignored: matches the --exclude regular expression
a.py already well formatted, good job.
some-dir/b.py already well formatted, good job. # < -- but not Black :/
All done! ✨ 🍰 ✨
2 files would be left unchanged.
This shouldn't be hard to fix but it will make the patch not as amazingly simple haha :)
This will also require changes to the .gitignore documentation we currently have. If you don't want to handle this part, I wouldn't mind writing the docs!
I'm also starting to really dislike the file collection / discovery tests we have. They need a fair bit of boilerplate and are just annoying to read. This is obviously out of scope for this PR, but I wanted to get it out there.
Thank you for sticking through with this feature! I'm looking forward to when this is merged.
Before fixing the bug: gitignore + get_gitignore(child) if gitignore else None, After: gitignore + get_gitignore(child) if gitignore is not None else None, yeah, it won't be as simple as the previous patch, lol! no problem, I updated the docs! Thanks for the review! |
+ I've allowed edits by maintainers so you can improve the testing data/methods before merging if you don't mind! Have a pythonic day! ❤️ |
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.
Pushed an explanatory comment since the if expression made no sense to me until I manually checked the value of gitignore
, but other than that LGTM. Thank you very much!
Have a pythonic day! ❤️
You too! 🐍
Nice, but before merging, I'm gonna modify the related tests! |
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.
Looks good, @ichard26 any reason not to merge yet?
Not sure though why Pipfile.lock
changed.
I just looked into it and the changes in I'll undo those changes and also fix the merge conflict and then I'll merge.
Other than the above, no. |
The original changes in Pipfile.lock are whitespace only. The changes turned the JSON's file indentation from 4 to 2. Effectively this happened: `json.dumps(json.loads(old_pipfile_lock), indent=2) + "\n"`. Just using main's Pipfile.lock instead of undoing the changes because 1) I don't know how to do that easily and quickly, and 2) there's a merge conflict.
conflicts for days ay?
Black
now respects.gitignore
files in all levels, not onlyroot/.gitignore
file (apply.gitignore
rules likegit
does).Fix: #1730