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

black does not exclude files in git submodules #3040

Open
janjachnik-dyson opened this issue Apr 28, 2022 · 8 comments
Open

black does not exclude files in git submodules #3040

janjachnik-dyson opened this issue Apr 28, 2022 · 8 comments
Labels
C: file collection Related to file collection (e.g. gitignore & cache) or file discovery and all of its configuration. T: bug Something isn't working

Comments

@janjachnik-dyson
Copy link

janjachnik-dyson commented Apr 28, 2022

Describe the bug

black does not exclude files in git submodules when full filename is passed

To Reproduce

I created an example project with a git submodule to test. The super-project has a pyproject.toml which excludes the subproject:

[tool.black]
force-exclude = 'subproject'

Commands to clone and reproduce bug:

git clone --recurse-submodules [email protected]:janjachnik-dyson/black-submodule-test-super.git
cd black-submodule-test-super
black --check subproject/file.py

The output:

Identified `/tmp/black-submodule-test-super/subproject` as project root containing a .git directory.
Sources to be formatted: "file.py"
subproject/file.py already well formatted, good job.

All done! ✨ 🍰 ✨
1 file would be left unchanged.

Expected behavior

subproject/file.py would be ignored because its path matches the force-exclude regex in pyproject.toml

Environment

  • Black's version: 22.3.0
  • OS and Python version: Ubuntu 20.04, Python 3.8.10

Additional context

We use black from within Visual Studio Code, where it is always called with the full filename. If black is manually run on the root folder of the super-project, the excludes are handled correctly. Within VSCode we pass the --config argument to specify the correct configuration file of the super-project.

Possible solution

The problem occurs because black decides the project root by recursively searching parent directories for .git, .hg and pyproject.toml files. In this case, the project root becomes the subproject folder (due to .git) and the exclude regex is compared against the path relative to the project root, which no longer contains the subproject string.

As a possible fix, I added the option to manually specify the project root:
janjachnik-dyson@6fc3368

Then, the following command works to properly exclude the file:

black --project-root . subproject/file.py

If this sounds like a valid fix/workaround, I'll raise a PR. Or is there a better way to handle excludes with git submodules?

@janjachnik-dyson janjachnik-dyson added the T: bug Something isn't working label Apr 28, 2022
@ichard26 ichard26 added the C: file collection Related to file collection (e.g. gitignore & cache) or file discovery and all of its configuration. label May 2, 2022
@edwardpeek-crown-public

We've also been seeing issues where black doesn't correctly handle .gitignore with submodules. In our case it's a top-level .gitignore being incorrectly applied to files in submodules, eg.

$ black -v .
Identified `...` as project root containing a .git directory.
Sources to be formatted: "."
example.py ignored: matches the .gitignore file content
submodule/example.py ignored: matches the .gitignore file content

It would be nice for black to match git behaviour 1:1 by default, without needing CLI workarounds.

@janjachnik-dyson
Copy link
Author

I've been trying to write a test for this and I'm concerned that the existing exclude tests do not reflect real-world usage.

The existing tests make use of the assert_collected_sources() function:

def assert_collected_sources(
which in-term makes a call to get_sources():
def get_sources(

However, the result of get_sources() is dependent on ctx.obj["root"]. That root folder is set dynamically when you run black using the find_project_root() function:

black/src/black/__init__.py

Lines 458 to 459 in 6c1bd08

root, method = find_project_root(src) if code is None else (None, None)
ctx.obj["root"] = root

However, in all of the include/exclude tests the root folder is manually set. None of the tests will reproduce the behaviour that occurs if find_project_root() finds a different folder, which is the behaviour when you actually use black.

@janjachnik-dyson
Copy link
Author

In my branch, I have added a test which reproduces the behaviour of this bug: https://github.com/janjachnik-dyson/black/tree/feature/manually_specify_project_root

The problem only seems to occur when the full file path is given, I added tests for both scenarios (base path and full path).

To make sure the tests are run correctly, I now set ctx.obj['root'] inside assert_collected_sources() by making a call to black.find_project_root() - this mimics the behaviour of the black executable. Without that change, the test would pass.

So now we have the bug reproduced in a test, I'd like some feedback/guidance on how to fix it.

I've suggested one solution in a draft PR here: #3116, which essentially gives the ability to manually override the project root.

Is my solution a viable option, or should the bug be fixed in a way that doesn't require manual intervention?

@darthwalsh
Copy link

I was expecting that black would never try to format files in any submodule, but maybe our team has a different setup than other python project. We have googletest as a submodule (i think is the normal way to use gtest), which seems to have python2 scripts which makes black fail. I could file a different feature-request for "black option to exclude files in git submodules"? (Or would that be a dupe of this issue?)

@derpda
Copy link

derpda commented Jan 12, 2023

Any news on this front? This is a major nuisance for us, would be great if a workaround was available.

@ichard26
Copy link
Collaborator

@derpda No update currently. I'm open to specifying the project root. I'll try to take a look at the linked PR soon-ish. A admittedly hacky workaround is to create an empty file at the root of your project and pass that file along with the file from the submodule.

black empty.py submodule/yourcode.py

This way Black will be forced to use the common parent of the two files as the project root.

@derpda
Copy link

derpda commented Jan 17, 2023

Thank you for getting back to me on this!
I tried this in VSCode, and it will sadly still format files that are in the force-exclude list.
Weird thing is though, if I copy the command from VSCode's Ouput window and execute it in the shell it works as expected, ignoring the second file if it is in the exclude list. When formatting on save however, it will apply changes.

That is probably beyond the scope of this issue, but if you (or someone else) knows something, that would be much appreciated!

@huntedman
Copy link

Still relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: file collection Related to file collection (e.g. gitignore & cache) or file discovery and all of its configuration. T: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants