Project root detection logic leads to configuration duplication in some circumstances #110
Comments
|
The current behavior is specifically designed with a monorepo in mind, though more from the perspective of embedding third party/OSS packages into a larger repo, where stopping at the nearest The former use case is one that I have a vested interest in maintaining; the latter is one that I'm sympathetic to, but haven't personally run into, and I'm not really sure how to find a good compromise between these two use cases, and their needs seem somewhat opposed. I'm open to ideas though. That said, I think that adding a |
cosinequanon
added a commit
to cosinequanon/ufmt
that referenced
this issue
Aug 10, 2023
For omnilib#110 there are use cases where projects might have multiple pyproject.toml files in the repo for various configurations but we want to store all the `ufmt`/`usort`/`black` configuration in one place. Currently there is no easy way to do this without calling `ufmt` directly on the root of the project which can be super expensive in large projects. This adds a new flag so you can specify the root. This works well for the general configuration but I want to point out one area where it does not work correctly, specifically with excludes. Say you have this set up ``` $ ls ** pyproject.toml a: a.py pyproject.toml b: b.py $ cat pyproject.toml [tool.ufmt] excludes = [ "/a/", ] ``` all other files are empty. Then if you format the root you would expect only `b` to be formatted. If you format the `a` directory then `a.py` will get formatted currently as well. ``` $ ufmt --debug check . DEBUG ufmt.core Checking /private/tmp/project/b/b.py ✨ 1 file already formatted ✨ $ ufmt --debug check a DEBUG ufmt.core Checking /private/tmp/project/a/a.py ✨ 1 file already formatted ✨ ``` works as expected. the issue is now with the new flag in this PR, specifying the root and formatting directory `a` doesn't do what we expect ``` (.venv) [/tmp/project]$ ufmt --root=. --debug check a DEBUG ufmt.core Checking /private/tmp/project/a/a.py ✨ 1 file already formatted ✨ ``` The issue is that overriding the `root` here is insufficient, we would also need to tell `Trailrunner` about it too, see https://github.com/omnilib/trailrunner/blob/main/trailrunner/core.py#L170 So this flag will help with some configuration but it might be confusing in terms of the excludes unless we also patch the other library to allow you to set the root.
cosinequanon
added a commit
to cosinequanon/ufmt
that referenced
this issue
Aug 10, 2023
For omnilib#110 there are use cases where projects might have multiple pyproject.toml files in the repo for various configurations but we want to store all the `ufmt`/`usort`/`black` configuration in one place. Currently there is no easy way to do this without calling `ufmt` directly on the root of the project which can be super expensive in large projects. This adds a new flag so you can specify the root. This works well for the general configuration but I want to point out one area where it does not work correctly, specifically with excludes. Say you have this set up ``` $ ls ** pyproject.toml a: a.py pyproject.toml b: b.py $ cat pyproject.toml [tool.ufmt] excludes = [ "/a/", ] ``` all other files are empty. Then if you format the root you would expect only `b` to be formatted. If you format the `a` directory then `a.py` will get formatted currently as well. ``` $ ufmt --debug check . DEBUG ufmt.core Checking /private/tmp/project/b/b.py ✨ 1 file already formatted ✨ $ ufmt --debug check a DEBUG ufmt.core Checking /private/tmp/project/a/a.py ✨ 1 file already formatted ✨ ``` works as expected. the issue is now with the new flag in this PR, specifying the root and formatting directory `a` doesn't do what we expect ``` (.venv) [/tmp/project]$ ufmt --root=. --debug check a DEBUG ufmt.core Checking /private/tmp/project/a/a.py ✨ 1 file already formatted ✨ ``` The issue is that overriding the `root` here is insufficient, we would also need to tell `Trailrunner` about it too, see https://github.com/omnilib/trailrunner/blob/main/trailrunner/core.py#L170 So this flag will help with some configuration but it might be confusing in terms of the excludes unless we also patch the other library to allow you to set the root.
amyreese
pushed a commit
to cosinequanon/ufmt
that referenced
this issue
Oct 19, 2023
For omnilib#110 there are use cases where projects might have multiple pyproject.toml files in the repo for various configurations but we want to store all the `ufmt`/`usort`/`black` configuration in one place. Currently there is no easy way to do this without calling `ufmt` directly on the root of the project which can be super expensive in large projects. This adds a new flag so you can specify the root. This works well for the general configuration but I want to point out one area where it does not work correctly, specifically with excludes. Say you have this set up ``` $ ls ** pyproject.toml a: a.py pyproject.toml b: b.py $ cat pyproject.toml [tool.ufmt] excludes = [ "/a/", ] ``` all other files are empty. Then if you format the root you would expect only `b` to be formatted. If you format the `a` directory then `a.py` will get formatted currently as well. ``` $ ufmt --debug check . DEBUG ufmt.core Checking /private/tmp/project/b/b.py ✨ 1 file already formatted ✨ $ ufmt --debug check a DEBUG ufmt.core Checking /private/tmp/project/a/a.py ✨ 1 file already formatted ✨ ``` works as expected. the issue is now with the new flag in this PR, specifying the root and formatting directory `a` doesn't do what we expect ``` (.venv) [/tmp/project]$ ufmt --root=. --debug check a DEBUG ufmt.core Checking /private/tmp/project/a/a.py ✨ 1 file already formatted ✨ ``` The issue is that overriding the `root` here is insufficient, we would also need to tell `Trailrunner` about it too, see https://github.com/omnilib/trailrunner/blob/main/trailrunner/core.py#L170 So this flag will help with some configuration but it might be confusing in terms of the excludes unless we also patch the other library to allow you to set the root.
amyreese
added a commit
that referenced
this issue
Oct 19, 2023
* Add `--root` flag to specify project configuration For #110 there are use cases where projects might have multiple pyproject.toml files in the repo for various configurations but we want to store all the `ufmt`/`usort`/`black` configuration in one place. Currently there is no easy way to do this without calling `ufmt` directly on the root of the project which can be super expensive in large projects. This adds a new flag so you can specify the root. This works well for the general configuration but I want to point out one area where it does not work correctly, specifically with excludes. Say you have this set up ``` $ ls ** pyproject.toml a: a.py pyproject.toml b: b.py $ cat pyproject.toml [tool.ufmt] excludes = [ "/a/", ] ``` all other files are empty. Then if you format the root you would expect only `b` to be formatted. If you format the `a` directory then `a.py` will get formatted currently as well. ``` $ ufmt --debug check . DEBUG ufmt.core Checking /private/tmp/project/b/b.py ✨ 1 file already formatted ✨ $ ufmt --debug check a DEBUG ufmt.core Checking /private/tmp/project/a/a.py ✨ 1 file already formatted ✨ ``` works as expected. the issue is now with the new flag in this PR, specifying the root and formatting directory `a` doesn't do what we expect ``` (.venv) [/tmp/project]$ ufmt --root=. --debug check a DEBUG ufmt.core Checking /private/tmp/project/a/a.py ✨ 1 file already formatted ✨ ``` The issue is that overriding the `root` here is insufficient, we would also need to tell `Trailrunner` about it too, see https://github.com/omnilib/trailrunner/blob/main/trailrunner/core.py#L170 So this flag will help with some configuration but it might be confusing in terms of the excludes unless we also patch the other library to allow you to set the root. * Use click.Path for validation --------- Co-authored-by: Amethyst Reese <amethyst@n7.gg>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently, μfmt appears to look for tool configuration in
pyproject.tomlfiles relative to the file under format, selecting the nearest ancestor. In a monorepo, or in a repo with a main package and several sub-packages, this would require duplicating black's and μsort's (and μfmt's) config in each individual package'spyproject.toml. Some alternatives I can think of are:cwd- this is a departure from how μfmt works right now so I assume you won't want to do thispyproject.tomlwithout a[tool.black]or[tool.usort]section, continue looking up the tree - if people do want to override the config from an outer folder they'll need to add a[tool.x]section in theirpyproject.tomlwhich is not the least bit intuitiveThanks for μfmt!
The text was updated successfully, but these errors were encountered: