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

Add --root flag to specify project configuration #160

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

cosinequanon
Copy link
Contributor

@cosinequanon cosinequanon commented Aug 10, 2023

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

$ 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
Copy link
Contributor Author

@amyreese could you please take a look, thanks!

@amyreese amyreese self-assigned this Oct 19, 2023
@amyreese amyreese added the enhancement New feature or request label Oct 19, 2023
cosinequanon and others added 2 commits October 18, 2023 21:08
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
Copy link
Member

Thank you, and sorry for the delay in reviewing this. I updated it to use the click.Path type for validating the values of --root, but otherwise looks great, and I appreciate your work on this.

re: trailrunner, if you're interested in a PR there to accept a root parameter to the Trailrunner constructor and plumb that through in µfmt, I'd be happy to review that as well.

Cheers! 🍻

@amyreese amyreese merged commit bf2aea6 into omnilib:main Oct 19, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants