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

Fix: black only respects the root gitignore. #2225

Merged
merged 10 commits into from May 16, 2021
Merged

Fix: black only respects the root gitignore. #2225

merged 10 commits into from May 16, 2021

Conversation

@hadialqattan
Copy link
Contributor

@hadialqattan hadialqattan commented May 11, 2021

Black now respects .gitignore files in all levels, not only root/.gitignore file (apply .gitignore rules like git does).

Fix: #1730

@hadialqattan
Copy link
Contributor Author

@hadialqattan hadialqattan commented May 11, 2021

I noticed that we've got a failure on the test cases out of this PR scope:

=================================== ERRORS ====================================
____________________ ERROR collecting tests/test_blackd.py ____________________
tests\test_blackd.py:10: in <module>
    import blackd
src\blackd\__init__.py:28: in <module>
    _stop_signal = asyncio.Event()
c:\hostedtoolcache\windows\python\3.8.9\x64\lib\asyncio\locks.py:260: in __init__
    self._loop = events.get_event_loop()
c:\hostedtoolcache\windows\python\3.8.9\x64\lib\asyncio\events.py:639: in get_event_loop
    raise RuntimeError('There is no current event loop in thread %r.'
E   RuntimeError: There is no current event loop in thread 'Dummy-1'.
____________________________ ERROR collecting gw0 _____________________________
Different tests were collected between gw1 and gw0. The difference is:
--- gw1

+++ gw0

@@ -101,19 +101,6 @@

For full log: check link

Loading

@ichard26
Copy link
Collaborator

@ichard26 ichard26 commented May 11, 2021

That's the price we pay for using pytest-xdist, sometimes we hit this bug: pytest-dev/pytest-xdist#620.

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.

Loading

@ichard26 ichard26 self-requested a review May 12, 2021
Copy link
Collaborator

@ichard26 ichard26 left a comment

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.

Loading

@hadialqattan
Copy link
Contributor Author

@hadialqattan hadialqattan commented May 14, 2021

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!

Loading

@hadialqattan hadialqattan requested a review from ichard26 May 14, 2021
@hadialqattan
Copy link
Contributor Author

@hadialqattan hadialqattan commented May 14, 2021

+ 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! ❤️

Loading

Copy link
Collaborator

@ichard26 ichard26 left a comment

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! 🐍

Loading

@hadialqattan
Copy link
Contributor Author

@hadialqattan hadialqattan commented May 15, 2021

Nice, but before merging, I'm gonna modify the related tests!

Loading

@hadialqattan hadialqattan requested a review from ichard26 May 15, 2021
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Looks good, @ichard26 any reason not to merge yet?

Not sure though why Pipfile.lock changed.

Loading

@ichard26
Copy link
Collaborator

@ichard26 ichard26 commented May 16, 2021

I just looked into it and the 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"

I'll undo those changes and also fix the merge conflict and then I'll merge.

ichard26 any reason not to merge yet?

Other than the above, no.

Loading

ichard26 added 2 commits May 16, 2021
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.
@ichard26 ichard26 merged commit b8450b9 into psf:main May 16, 2021
33 checks passed
Loading
@hadialqattan hadialqattan deleted the i1730 branch May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants