New dev setup

[cgahr]

Hey everyone,

I saw some comments regarding improving the dev setup and moving all configuration to pyproject.toml. Here is my take on that:

add pre-commit hooks

ssort introduces a pre-commit hook, so why not pre-commit to improve the code.

These are the hooks I propose to add (through trial and error I found those to be useful):

little impact

• check-added-large-files: checks that you don't add files that should not be there
• end-of-file-fixer: enforce newline at end of file
• trailing-whitespace: remove trailing whitespaces
• check-toml: ensure that pyproject.toml is parsable
• toml-sort: enfore that pyproject.toml is sorted

already existed

• black: runs black after each commit to enforce consistent formatting
• mypy: runs mypy after each commit

big impact:

• pytest: run all tests after each commit. This hook is not working, I'd need some input how to run the tests properly.

Altogether, these hooks seem to be in the spirit of the existing tools used.

run code on existing files

I ran all hooks on the existing files, there were 2 small changes

add config to exclude test_data from black and isort

As far as I can see, the test_data folder contains example code to test ssort on. As such, black shouldn't format and mypy should not run on this code. This commit ensures that.

add toml sort config

enforce a consistent sorting of pyproject.toml

convert setup.cfg and setup.py to pyproject.toml

The main commit:

• remove setup.cfg

• add package metadata to pyproject.toml
  As far as I checked, all metadata should be correct. Installing the package works aswell.

• remove setup.py: setup.py seems to be not necessary for installing the package correctly. However, the tox tests seem to fail now.

ToDo:

Basically, fix the tests. This includes deciding whether to run the tests using pre-commit and how to run the test properly. For both points I need your input.

Constantin

[bwhmather]

Hi Constantin - Thanks for this.

How are you running the tests? What OS/python version?

Interested in all of the linting fixes, although would prefer that CI be at least as strict as local testing to avoid situations where someone bypasses checks and then later committers have to clean it up.

setup.cfg/setup.py also good.

For running tests/tools, I would prefer to stick with tox. Have been a happy user since 2011, and much prefer its explicitness to pre-commit's promise of stuff magically happening in the background.

Will see if I can bump the CI test runners as it looks like ubuntu 18.04 was deprecated late last year.

Are you interested in making the fix for the __path__ bug you reported earlier?

[jgberry]

pytest: run all tests after each commit. This hook is not working, I'd need some input how to run the tests properly.

I'm not so sure running the entire test suite on each commit is preferable. There are speed concerns, as mentioned here, but on top of that, making intermediate commits which do not pass the test suite should be perfectly acceptable. Perhaps, if not solely for the sake of keeping this PR as simple as possible, this behavior can be omitted from this PR and reevaluated in a later issue or PR.

Another concern I have, which is not really the fault of this PR but more so of our CI infrastructure in general, is that it seems like it will become increasingly difficult to maintain all of this CI. With this addition, there will be three different locations where CI will need to be maintained: tox.ini, .pre-commit-config.yaml, and .github. With all of these CI mechanisms using their own logic (very little logic reuse between the three), I find it very likely one or more of these files will become unmaintained. Maybe we should explore trying to combine these approaches as much as possible to avoid any such situation. I think something along the lines of github calls tox which calls pre-commit seems pretty reasonable to me.

[jgberry]

Basically, fix the tests. This includes deciding whether to run the tests using pre-commit and how to run the test properly. For both points I need your input.

Tests are run with tox. You'll need to add isolated_build=true to the tox.ini for PEP 517 compatibility. There are also still a number of dangling references to setup.py laying around the project which should be cleaned up (lots of references in .github, one in the README.rst, and a bunch in tox.ini).

[jgberry]

I'm also of the opinion that this should be broken into two PRs: one for adding the pyproject.toml and one for adding the .pre-commit-config.yaml. These are really two different concerns that are vaguely related.

[cgahr]

So, to summarize: I'll make 2 PRs, one for moving setup.* to pyproject.toml. And then we have another discussion regarding if and how to change the dev setup.

[cgahr]

Basically, fix the tests. This includes deciding whether to run the tests using pre-commit and how to run the test properly. For both points I need your input.

Tests are run with tox. You'll need to add isolated_build=true to the tox.ini for PEP 517 compatibility. There are also still a number of dangling references to setup.py laying around the project which should be cleaned up (lots of references in .github, one in the README.rst, and a bunch in tox.ini).

These are all fixed in PR #81

[cgahr]

I'm not so sure running the entire test suite on each commit is preferable. Thttps://github.com/pre-commit/pre-commit-hooks/issues/291#issuecomment-394167917 are speed concerns, as mentioned here, but on top of that, making intermediate commits which do not pass the test suite should be perfectly acceptable. Perhaps, if not solely for the sake of keeping this PR as simple as possible, this behavior can be omitted from this PR and reevaluated in a later issue or PR.

You are probably correct, it does not make sense to run the tests as a git hook.

Another concern I have, which is not really the fault of this PR but more so of our CI infrastructure in general, is that it seems like it will become increasingly difficult to maintain all of this CI. With this addition, there will be three different locations where CI will need to be maintained: tox.ini, .pre-commit-config.yaml, and .github. With all of these CI mechanisms using their own logic (very little logic reuse between the three), I find it very likely one or more of these files will become unmaintained. Maybe we should explore trying to combine these approaches as much as possible to avoid any such situation. I think something along the lines of github calls tox which calls pre-commit seems pretty reasonable to me.

First of all #81 simplifies the configuration a bit by moving black's, isort's, and mypy's common elements in tox.ini and github ci to pyproject.toml.

I think the main question is, what do you want to have? Looking at tox.ini I think it configures tests and configures different linters. The question is now do you want to run all those using tox?

For running tests/tools, I would prefer to stick with tox. Have been a happy user since 2011, and much prefer its explicitness to pre-commit's promise of stuff magically happening in the background.

The answer seems to be no.

I'd propose something different:

• use tox to run the tests only
• use pre-commit to run linters like black, isort, ...
• use github ci to check everything

My reasoning for that is the following: for me, installing tox environments never works correctly and I'm always missing something. On the other hand, pre-commit just works.

In addition, it is really annoying if github ci fails, because you did not run black or isort and pre-commit kind of forces you to run them.

What do you think?

[bwhmather]

I think something along the lines of github calls tox which calls pre-commit seems pretty reasonable to me.

My view on this is that tox, github CI and pre-commit are all at about the same level of abstraction and so shouldn't depend on each other. I think the correct approach is to move as much as possible to the next layer down. Moving exclude rules to pyproject.toml is a good example (thank you @cgahr). Perhaps installing tools using dev-dependencies rather than separately would be another easy win?

[bwhmather]

I think the main question is, what do you want to have? Looking at tox.ini I think it configures tests and configures different linters. The question is now do you want to run all those using tox?

I think the answer is that we want to either run everything using tox or everything using pre-commit. Both are currently completely adequate for the job, and using two just makes it twice as likely that something will break.

Weirdly I don't think we can make the decision based on current features. Given that this project is mostly just ticking over with regular but infrequent updates to handle new python versions and occasional feature requests, the most important factor is how scary it is to come back to after six months.

My expectation with tox is that it will be broken by the latest PEP, I'll have an obvious stack trace, and I'll have to do a little bit of reading to fix it

My suspicion with pre-commit is that it will magically auto-update and everything will appear to run fine, but what it is running will no longer be what I audited.

[bwhmather]

In addition, it is really annoying if github ci fails, because you did not run black or isort and pre-commit kind of forces you to run them.

To me, the most important thing is that failures should be a result of changes in the current PR. Having local tests that are stricter than CI tests makes it more likely that someone will bypass the local checks and break subsequent pushes. The well behaved developer who actually runs the tests locally then has the awkward choice between raising a separate PR to fix the previous break, bundling the unrelated fixes with the current PR, or bypassing local tests. The developer who broke the local tests doesn't see the consequences.