add support for pyproject.toml #84

cgahr · 2023-04-26T14:29:12Z

TODO:

change root discovery, individually named files should have individual roots
throw an error on non-existent files
ignore files in .gitignore
rewrite test_config.py tests to fit the style of the other tests
restore old/removed tests to check if the behavior remained the same
improve fixture usage in test_files.py

I open this PR to track the changes I introduced to support pyproject.toml configuration for ssort.

~~This PR depends on #81. After merging #81, the first few commits in this PR should disappear since I already merged all changes from #81 into this PR.~~

This PR fixes #72, #62, #9.

Still missing:

tests for _config.py
restore the removed error handling in _main.py and its tests (not necessary anymore)
add content of .gitignore to excluded files (should this be configurable, i.e. use_gitignore=true?) -> not now, maybe later

The code works as follows:

_files.py implements the logic to find the root of a project
if a pyproject.toml exists in the root folder, _config.py parses it and creates a config object. If pyproject.toml does not exist, the config object is populated with sensible defaults.
the config class then provides the iterate_files_matching_pattern iterator that iterates over all files matching the pattern provided and the configuration.
by default files that are passed explicitly by their full name are always sorted even if they are ignored in the config. That mirror black's default behavior.

pyproject.toml

For now, the following keys are supported. They are inspired by black's and pylint's configuration:

[tool.ssort]
# skip = []
extend_skip = ['unsortable']
skip_glob = ['src/**/ugly.py']

The skip key contains sensible defaults for files that should be skipped. Changing skip overwrites those defaults.
extend_skip can be used to extend the files skipped in skip. In this case the files and folders named unsortable are skipped.
skip_glob parses glob expressions to skip files. In this case, all files named ugly.py located somewhere under src are not sorted.

What do you think?

…all valid python files

…oml beyond the specified test-folder

…roject.toml

…a file

bwhmather · 2023-05-03T19:05:07Z

tests/test_executable.py

-        f"ERROR: {escape_path(path)} does not exist\n",
-        "1 file was not sortable\n",
-    ]
-    expected_status = 1


I think that the old behaviour is preferable. If files are explicitly named then they should be sorted. If they can't be sorted then it should be reported as an error.

bwhmather · 2023-05-03T19:25:39Z

src/ssort/_files.py

-            if subpath not in paths_set:
-                paths_set.add(subpath)
-                yield subpath
+    return common_base


I think it's important for the git root to be per-path. If it isn't then the results of running ssort ./repo1/ ./repo2/ won't match the results of running ssort ./repo1; ssort ./repo2/. Having a single git root will also break with git submodules.

bwhmather · 2023-05-03T19:32:51Z

src/ssort/_files.py


+def find_project_root(patterns):


I think we need to track the .git root and the pyproject.toml root separately.

We want .gitigore files outside the pyproject.toml root to be respected but inside the .git root, but we want project.toml files outside the .git root to be ignored.

bwhmather · 2023-05-03T19:34:52Z

tests/test_config.py

+    assert hasattr(config, "DEFAULT_SKIP")
+
+
+class TestIterValidPythonFiles:


For consistency, would it be possible to rewrite these tests using the pytest test_ function style..

bwhmather · 2023-05-03T19:41:46Z

tests/test_files.py

-    (tmp_path / "link2").symlink_to(tmp_path / "link1")
-
-    assert not is_ignored("link1")
-    assert not is_ignored("link2")


Would it be possible to restore these tests so that we can see whether the current no-pyproject.toml behaviour has changed?

bwhmather · 2023-05-03T19:43:44Z

tests/test_files.py

+    def neither(self, tmp_path):
+        root = tmp_path / "root"
+        root.mkdir()
+        return root


I don't think that having this code in fixtures is helpful. At best, it adds a layer of indirection with no abstraction. At worst, it encourages coupling. A variable binding provides equivalent abstraction with no indirection.

Here my intention was to have 3 fixtures, one for find a .git folder, one for finding a pyproject.toml file and lastly finding neither.

I see where the confusion is! I was referring specifically to the pytest.fixture when I said fixture. I agree that these are fixtures, but that doesn't mean that they have to be pytest.fixtures.

I would argue that there are at least two sorts of fixtures:

Fixtures which provide the background in which your test runs: e.g. tmp_path or references to a default-initialised database.

Fixtures which set the specific state against which you want to test: e.g. specific files with in a temporary directory, or particular rows in a database.

There are also other things which use pytest.fixture but that aren't fixtures: pytest.monkeypatch is a good example as it just uses pytest.fixture for lifecycle management but doesn't set up any state.

Type 1 fixtures are just background and should be attract as little attention as possible when added to a test. They should usually be defined outside the test and can often be shared.

Type 2 fixtures are core to understanding what a test is asserting and so it makes sense for them to be inline and explicit. Changing them changes what all tests that use them assert so sharing them is dangerous but it may be possible to share some code to construct them.

To my mind, these are definitely type 2 fixtures and so should be inline.

cgahr · 2023-05-05T13:21:04Z

Thanks for your feedback.

which pyproject.toml to find and determining the project root

Consider the following project:

- pyproject.toml
- dir1:
   - pyproject.toml
   - file.py
- dir2
   - pyproject.toml
   - file.py

How should the discovery of pyproject.toml work?

I based the behavior of my implementation of black:

black allows only one pyproject.toml per execution of black. This means
- black .
- black dir1
- black dir2
produces different ressults, depending on the respective config.
black ignores the content of .gitignore by default, I will change that back.
my implementation of the root detection is similar to blacks version

IMO, these defaults seem to be sensible, and they have already passed the test of time.
So my question is:

Will ssort follow the example of black or will it do its own thing?

handle missing files

Lastly, regarding throwing an error if a filepath is passed that does not exist. The
default of black is to exit immediatelly and not run its code. Do we want a similar
behavior? Right now, commands like

ssort dir1/doesnt_exist.py

don't throw an error and exit with error code 0 (in contrast to blacks behavior).

If we want to change this behavior, it might make sense to switch from argparse to
click (mirroring black again) and check while parsing the input paths, if the all
exist.

What do you think?

bwhmather · 2023-05-06T07:14:21Z

Right now, commands like
ssort dir1/doesnt_exist.py
don't throw an error and exit with error code 0 (in contrast to blacks behavior).

That's odd. This should be an error.

bwhmather · 2023-05-06T07:17:26Z

If we want to change this behavior, it might make sense to switch from argparse to
click (mirroring black again) and check while parsing the input paths, if the all
exist.

We need to validate at read time anyway to handle race conditions. More consistent to just have one place where we check. Also, would prefer not to add any more external dependencies than we absolutely need.

bwhmather · 2023-05-06T07:22:18Z

Will ssort follow the example of black or will it do its own thing?

This is a very good question. I think the ssort model of having the roots used for determining what rules to apply being determined based on the file rather than the pwd is safer and less likely to cause surprise so I am reluctant to change it. Perhaps this is a discussion worth having with the black developers?

cgahr · 2023-05-11T21:04:01Z

Right now, commands like
ssort dir1/doesnt_exist.py
don't throw an error and exit with error code 0 (in contrast to blacks behavior).
That's odd. This should be an error.

I meant that ssort still has exit code 0, so technically ssort did not throw an error.

cgahr · 2023-05-11T21:05:15Z

What discussion do you think should be had?

bwhmather · 2023-05-11T21:13:06Z

I meant that ssort still has exit code 0, so technically ssort did not throw an error.

That, to me, is a bug in ssort. We should probably fix it.

bwhmather · 2023-05-11T21:20:57Z

What discussion do you think should be had?

Politely suggest that they adopt ssorts model of having the project root be per matching file. See what they say. Respond accordingly.

Suspect that they would come back with one of three responses:

No. Doing so would break backward compatibility.
No. Black's model is better. Here are the reasons why.
Yes.

If 2, we update ssort. If 1, someone hopefully factors the ssort logic out into a library that supports pyproject.toml/.gitignore/.hgignore/... and eventually black picks it up. If 3, we've made the lives of lots of python developers ever so slightly better.

cgahr · 2023-05-12T14:18:22Z

I'll open such an issue, but, to be completely honest, I don't think they'll change their way and I'm also not 100% behind this logic.

In my opinion, there should be one, and only one, point of configuration per project. With this way of thinking our discussion is kind of a moot point.

In addition (also in my opinion), the default way of running ssort is ssort . from the project root, which makes our discussion a moot point again.

In any case, one possible workaround could be the following:

use blacks logic of discovering the configuration
if ssort is run with multiple files, i.e. ssort file.py dir/file.py, we internally run ssort for each file individually.

Constantin Gahr added 27 commits April 25, 2023 09:14

exclude test_data/samples/* from black, isort, mypy

1a671bb

remove black, isort, mypy config from tox.ini

dbce624

remove black, isort, mypy config from github ci

056f6e2

remove testing of setup.py in tox.ini

ac2840c

remove setup.py

5b8776c

move config from setup.cfg to pyproject.toml

d50431a

remove mention of setup.py in readme

0c8cf68

update tox.ini for PEP517 compatibility

556e674

Merge branch 'master' into pyproject-toml

f4c9aae

update release ci to not use setup.py

8c4190a

remove setup.py from readme

aa7f40b

Merge branch 'master' into fix-72

e1088e0

introduce Config class, that parses pyproject.toml and iterates over …

b5456a8

…all valid python files

remove redundant find_python_files code, add find_project_root

f6439b9

use new config to iterate over python files

fa2b99f

remove unnecessary error handling, both are handled by config.files()

193af08

add config for ssort

5080f51

update tox.ini for PEP517 compatibility

3955ad8

Merge branch 'pyproject-toml' into fix-72

ce73311

update tests for _files.py

3d55df1

fix bug find_project_root

3b91f35

use tomli for python < 3.11, tomlib for python >= 3.11

6a659df

add tomli dependency

797424b

remove pathspec mypy block since pathspec is not longer needed

cafa5bb

remove test to check for not existent file, this cannot happen anymore

674ef12

mock find_project_root so that pytest does not search for pyproject.t…

91d8317

…oml beyond the specified test-folder

ssort only sorts the inteded files, not all files under root

192d9ec

cgahr marked this pull request as draft April 26, 2023 14:30

Constantin Gahr added 2 commits April 27, 2023 09:35

Merge branch 'master' into fix-72

be890a4

Merge branch 'master' into fix-72

f18cee6

cgahr marked this pull request as ready for review April 30, 2023 12:04

This was referenced Apr 30, 2023

Make it possible to exclude files from reformatting #9

Open

Add pyproject.toml / setup.py configuration #72

Open

Explicitly choose to run on all files #62

Open

Constantin Gahr added 9 commits April 30, 2023 17:54

export only get_config_from_root using __all__

d65d304

fix import statement to mirror the others

e9689b5

add current working dir to find_project_root to find the expected pyp…

08cb4d7

…roject.toml

add skip_glob configuration key to filter files with glob pattern

6bbf597

add tests for Config.is_invalid

ad3ff91

fix a bug where glob pattern was incorrectly applied if path was not …

8c049d1

…a file

create current_working_dir to make testing easier, add more tests

ee33e99

remove __pycache__ and .pytest_cache to match blacks defaults

77e6705

ignore __pycache__ and .pytest_cache folders

4440ec6

This was referenced May 1, 2023

Refactor step 1: remove _utily.py #91

Open

Refactor code #92

Open

bwhmather reviewed May 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for pyproject.toml #84

add support for pyproject.toml #84

cgahr commented Apr 26, 2023 •

edited

bwhmather May 3, 2023

bwhmather May 3, 2023

bwhmather May 3, 2023

bwhmather May 3, 2023 •

edited

bwhmather May 3, 2023

bwhmather May 3, 2023

cgahr May 5, 2023

bwhmather May 6, 2023

cgahr commented May 5, 2023

bwhmather commented May 6, 2023

bwhmather commented May 6, 2023

bwhmather commented May 6, 2023

cgahr commented May 11, 2023

cgahr commented May 11, 2023

bwhmather commented May 11, 2023

bwhmather commented May 11, 2023

cgahr commented May 12, 2023

		assert hasattr(config, "DEFAULT_SKIP")


		class TestIterValidPythonFiles:

add support for pyproject.toml #84

Are you sure you want to change the base?

add support for pyproject.toml #84

Conversation

cgahr commented Apr 26, 2023 • edited

pyproject.toml

bwhmather May 3, 2023

Choose a reason for hiding this comment

bwhmather May 3, 2023

Choose a reason for hiding this comment

bwhmather May 3, 2023

Choose a reason for hiding this comment

bwhmather May 3, 2023 • edited

Choose a reason for hiding this comment

bwhmather May 3, 2023

Choose a reason for hiding this comment

bwhmather May 3, 2023

Choose a reason for hiding this comment

cgahr May 5, 2023

Choose a reason for hiding this comment

bwhmather May 6, 2023

Choose a reason for hiding this comment

cgahr commented May 5, 2023

which pyproject.toml to find and determining the project root

handle missing files

bwhmather commented May 6, 2023

bwhmather commented May 6, 2023

bwhmather commented May 6, 2023

cgahr commented May 11, 2023

cgahr commented May 11, 2023

bwhmather commented May 11, 2023

bwhmather commented May 11, 2023

cgahr commented May 12, 2023

cgahr commented Apr 26, 2023 •

edited

bwhmather May 3, 2023 •

edited