Skip to content

Commit

Permalink
Add --root flag to specify project configuration
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
cosinequanon committed Aug 10, 2023
1 parent 76b903a commit 05e2e85
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 20 deletions.
30 changes: 26 additions & 4 deletions ufmt/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,28 @@ def f(v: int) -> str:
default=None,
help="Override the default concurrency",
)
def main(ctx: click.Context, debug: Optional[bool], concurrency: Optional[int]):
@click.option(
"--root",
type=str,
default=None,
help="Specify the root directory for project configuration",
)
def main(
ctx: click.Context,
debug: Optional[bool],
concurrency: Optional[int],
root: Optional[str],
):
init_logging(debug=debug)
root_dir = Path(root) if root is not None else None
if root_dir is not None and not root_dir.is_dir():
raise ValueError("Root must be a valid directory")

ctx.obj = Options(
debug=debug is True,
quiet=debug is False,
concurrency=concurrency,
root=root_dir,
)
enable_libcst_native()

Expand All @@ -123,7 +139,9 @@ def check(ctx: click.Context, names: List[str]):
"""Check formatting of one or more paths"""
options: Options = ctx.obj
paths = [Path(name) for name in names] if names else [Path(".")]
results = ufmt_paths(paths, dry_run=True, concurrency=options.concurrency)
results = ufmt_paths(
paths, dry_run=True, concurrency=options.concurrency, root=options.root
)
changed, error = echo_results(results, quiet=options.quiet)
if changed or error:
ctx.exit(1)
Expand All @@ -139,7 +157,11 @@ def diff(ctx: click.Context, names: List[str]):
options: Options = ctx.obj
paths = [Path(name) for name in names] if names else [Path(".")]
results = ufmt_paths(
paths, dry_run=True, diff=True, concurrency=options.concurrency
paths,
dry_run=True,
diff=True,
concurrency=options.concurrency,
root=options.root,
)
changed, error = echo_results(results, diff=True, quiet=options.quiet)
if changed or error:
Expand All @@ -155,7 +177,7 @@ def format(ctx: click.Context, names: List[str]):
"""Format one or more paths in place"""
options: Options = ctx.obj
paths = [Path(name) for name in names] if names else [Path(".")]
results = ufmt_paths(paths, concurrency=options.concurrency)
results = ufmt_paths(paths, concurrency=options.concurrency, root=options.root)
_, error = echo_results(results, quiet=options.quiet)
if error:
ctx.exit(1)
5 changes: 3 additions & 2 deletions ufmt/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ class UfmtConfig:
excludes: List[str] = field(default_factory=list)


def ufmt_config(path: Optional[Path] = None) -> UfmtConfig:
def ufmt_config(path: Optional[Path] = None, root: Optional[Path] = None) -> UfmtConfig:
path = path or Path.cwd()
root = project_root(path)
if root is None:
root = project_root(path)
config_path = root / "pyproject.toml"
if config_path.is_file():
pyproject = tomlkit.loads(config_path.read_text())
Expand Down
3 changes: 2 additions & 1 deletion ufmt/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ def ufmt_paths(
pre_processor: Optional[Processor] = None,
post_processor: Optional[Processor] = None,
concurrency: Optional[int] = None,
root: Optional[Path] = None,
) -> Generator[Result, None, None]:
"""
Format one or more paths, recursively, ignoring any files excluded by configuration.
Expand Down Expand Up @@ -375,7 +376,7 @@ def ufmt_paths(
if path == STDIN:
LOG.warning("Cannot mix stdin ('-') with normal paths, ignoring")
continue
config = ufmt_config(path)
config = ufmt_config(path, root)
all_paths.extend(runner.walk(path, excludes=config.excludes))

if not all_paths:
Expand Down
54 changes: 41 additions & 13 deletions ufmt/tests/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,9 @@ def test_check(self, ufmt_mock):
ufmt_mock.reset_mock()
ufmt_mock.return_value = []
result = self.runner.invoke(main, ["check"])
ufmt_mock.assert_called_with([Path(".")], dry_run=True, concurrency=None)
ufmt_mock.assert_called_with(
[Path(".")], dry_run=True, concurrency=None, root=None
)
self.assertRegex(result.stderr, r"No files found")
self.assertEqual(0, result.exit_code)

Expand All @@ -117,7 +119,10 @@ def test_check(self, ufmt_mock):
]
result = self.runner.invoke(main, ["check", "bar.py", "foo/frob.py"])
ufmt_mock.assert_called_with(
[Path("bar.py"), Path("foo/frob.py")], dry_run=True, concurrency=None
[Path("bar.py"), Path("foo/frob.py")],
dry_run=True,
concurrency=None,
root=None,
)
self.assertEqual(0, result.exit_code)

Expand All @@ -129,7 +134,10 @@ def test_check(self, ufmt_mock):
]
result = self.runner.invoke(main, ["check", "bar.py", "foo/frob.py"])
ufmt_mock.assert_called_with(
[Path("bar.py"), Path("foo/frob.py")], dry_run=True, concurrency=None
[Path("bar.py"), Path("foo/frob.py")],
dry_run=True,
concurrency=None,
root=None,
)
self.assertEqual(1, result.exit_code)

Expand All @@ -149,7 +157,10 @@ def test_check(self, ufmt_mock):
]
result = self.runner.invoke(main, ["check", "bar.py", "foo/frob.py"])
ufmt_mock.assert_called_with(
[Path("bar.py"), Path("foo/frob.py")], dry_run=True, concurrency=None
[Path("bar.py"), Path("foo/frob.py")],
dry_run=True,
concurrency=None,
root=None,
)
self.assertRegex(
result.stderr, r"Error formatting .*frob\.py: Syntax Error @ 4:16"
Expand All @@ -163,7 +174,7 @@ def test_check(self, ufmt_mock):
]
result = self.runner.invoke(main, ["check", "foo.py"])
ufmt_mock.assert_called_with(
[Path("foo.py")], dry_run=True, concurrency=None
[Path("foo.py")], dry_run=True, concurrency=None, root=None
)
self.assertRegex(result.stderr, r"Skipped .*foo\.py: special")
self.assertEqual(0, result.exit_code)
Expand All @@ -175,7 +186,7 @@ def test_diff(self, ufmt_mock):
ufmt_mock.return_value = []
result = self.runner.invoke(main, ["diff"])
ufmt_mock.assert_called_with(
[Path(".")], dry_run=True, diff=True, concurrency=None
[Path(".")], dry_run=True, diff=True, concurrency=None, root=None
)
self.assertRegex(result.stderr, r"No files found")
self.assertEqual(0, result.exit_code)
Expand All @@ -192,6 +203,7 @@ def test_diff(self, ufmt_mock):
dry_run=True,
diff=True,
concurrency=None,
root=None,
)
self.assertEqual(0, result.exit_code)

Expand All @@ -207,6 +219,7 @@ def test_diff(self, ufmt_mock):
dry_run=True,
diff=True,
concurrency=None,
root=None,
)
self.assertEqual(1, result.exit_code)

Expand All @@ -230,6 +243,7 @@ def test_diff(self, ufmt_mock):
dry_run=True,
diff=True,
concurrency=None,
root=None,
)
self.assertRegex(
result.stderr, r"Error formatting .*frob\.py: Syntax Error @ 4:16"
Expand All @@ -243,7 +257,7 @@ def test_diff(self, ufmt_mock):
]
result = self.runner.invoke(main, ["diff", "foo.py"])
ufmt_mock.assert_called_with(
[Path("foo.py")], dry_run=True, diff=True, concurrency=None
[Path("foo.py")], dry_run=True, diff=True, concurrency=None, root=None
)
self.assertRegex(result.stderr, r"Skipped .*foo\.py: special")
self.assertEqual(0, result.exit_code)
Expand All @@ -255,18 +269,32 @@ def test_diff(self, ufmt_mock):
]
result = self.runner.invoke(main, ["--quiet", "diff", "foo.py"])
ufmt_mock.assert_called_with(
[Path("foo.py")], dry_run=True, diff=True, concurrency=None
[Path("foo.py")], dry_run=True, diff=True, concurrency=None, root=None
)
self.assertEqual("", result.stderr)
self.assertEqual(0, result.exit_code)

with self.subTest("bad root dir"):
ufmt_mock.reset_mock()
ufmt_mock.return_value = [
Result(Path("bar.py"), changed=False),
]
result = self.runner.invoke(
main, ["--root", "DOES_NOT_EXIST", "diff", "bar.py"]
)
self.assertRegex(
result.exception.args[0],
r"Root must be a valid directory",
)
self.assertEqual(1, result.exit_code)

@patch("ufmt.cli.ufmt_paths")
def test_format(self, ufmt_mock):
with self.subTest("no paths given"):
ufmt_mock.reset_mock()
ufmt_mock.return_value = []
result = self.runner.invoke(main, ["format"])
ufmt_mock.assert_called_with([Path(".")], concurrency=None)
ufmt_mock.assert_called_with([Path(".")], concurrency=None, root=None)
self.assertRegex(result.stderr, r"No files found")
self.assertEqual(0, result.exit_code)

Expand All @@ -278,7 +306,7 @@ def test_format(self, ufmt_mock):
]
result = self.runner.invoke(main, ["format", "bar.py", "foo/frob.py"])
ufmt_mock.assert_called_with(
[Path("bar.py"), Path("foo/frob.py")], concurrency=None
[Path("bar.py"), Path("foo/frob.py")], concurrency=None, root=None
)
self.assertEqual(0, result.exit_code)

Expand All @@ -290,7 +318,7 @@ def test_format(self, ufmt_mock):
]
result = self.runner.invoke(main, ["format", "bar.py", "foo/frob.py"])
ufmt_mock.assert_called_with(
[Path("bar.py"), Path("foo/frob.py")], concurrency=None
[Path("bar.py"), Path("foo/frob.py")], concurrency=None, root=None
)
self.assertEqual(0, result.exit_code)

Expand All @@ -310,7 +338,7 @@ def test_format(self, ufmt_mock):
]
result = self.runner.invoke(main, ["format", "bar.py", "foo/frob.py"])
ufmt_mock.assert_called_with(
[Path("bar.py"), Path("foo/frob.py")], concurrency=None
[Path("bar.py"), Path("foo/frob.py")], concurrency=None, root=None
)
self.assertRegex(
result.stderr, r"Error formatting .*frob\.py: Syntax Error @ 4:16"
Expand All @@ -323,7 +351,7 @@ def test_format(self, ufmt_mock):
Result(Path("foo.py"), skipped="special"),
]
result = self.runner.invoke(main, ["format", "foo.py"])
ufmt_mock.assert_called_with([Path("foo.py")], concurrency=None)
ufmt_mock.assert_called_with([Path("foo.py")], concurrency=None, root=None)
self.assertRegex(result.stderr, r"Skipped .*foo\.py: special")
self.assertEqual(0, result.exit_code)

Expand Down
11 changes: 11 additions & 0 deletions ufmt/tests/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,17 @@ def test_ufmt_config(self):
config,
)

with self.subTest("absolute path, manually specify project root"):
config = ufmt_config(root=self.td)
self.assertEqual(
UfmtConfig(
project_root=self.td,
pyproject_path=self.pyproject,
excludes=["a", "b"],
),
config,
)

@patch("ufmt.config.LOG")
def test_invalid_config(self, log_mock):
with self.subTest("string"):
Expand Down
1 change: 1 addition & 0 deletions ufmt/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class Options:
debug: bool = False
quiet: bool = False
concurrency: Optional[int] = None
root: Optional[Path] = None


class Processor(Protocol):
Expand Down

0 comments on commit 05e2e85

Please sign in to comment.