drop isort and black from pyproject.toml

[finswimmer]

Drop isort and black from pyproject.toml, because these linter are running via pre-commit.

[dimbleby]

Just seen that you opened an MR. Per #274 (comment) I don't know that I think this is a good idea; but since we have this MR it makes sense to continue any discussion here rather than there.

[dimbleby]

It's desirable to run tools such as mypy and isort in the virtual environment of the project. This project's precommit configuration currently fails to do this. This MR is IMO a step in the wrong direction.

https://github.com/pre-commit/mirrors-mypy suggests that the way to get the dependencies you want in place for the mypy precommit is by repeating them in precommit configuration

Because pre-commit runs mypy from an isolated virtualenv (without your dependencies) you may also find it useful to add the typed dependencies to additional_dependencies so mypy can better perform dynamic analysis:

That duplication with pyproject.toml is clearly undesirable. I'd propose that pyproject.toml should be the source of truth for creating the environment in which development and testing happens, and pipelines should use that.

(This seems to be slightly contentious in the precommit world, I found a couple of tetchy threads. I probably don't understand the issues.)

I think the way to do that would be to put language: system somewhere in your precommit configuration and then take responsibility yourself for running (eg) mypy in the correct environment. For this project that would presumably be a matter of going poetry run mypy. ie move away from https://github.com/pre-commit/mirrors-mypy altogether.

[danieleades]

there are good reasons to remove these pre-commit dependencies from the pyproject.toml file in some projects. The extra dependencies place additional constraints on the version resolution, and it's possible to have a project where version resolution fails with the development dependencies, but works perfectly without them. I've run into this myself, generally on projects trying to pair old dependencies with recent dev dependencies. I've not seen any evidence that this impacts Poetry at all, but it's an edge case to be aware of, and one that you avoid by using pre-commit to manage these dependencies.

On the other hand having pyproject.toml as the source of truth has some advantages

• a single source of truth for all dependencies
• linters are available to the IDE

There's some tension between the two trade-offs.

I agree with the sentiment of this PR though- having them duplicated in both places is problematic.

[finswimmer]

We should keep it easy for contributors. Telling them to run pre-commit, mypy, something-else before a commit makes it harder. That's another reason beside the goal having one single source of truth for the linting stuff.

I agree that this isn't ideal for mypy, since it may need to know about dependencies of the package. isort and black doesn't have this need.

linters are available to the IDE

Any IDE that is able to run external commands, can run the linter via pre-commit as well, e.g. pre-commit run black runs only black. With the --files parameter one can specify on which file it should run. PyCharm for example have the variable $FilePath$ on can use for the current open file.

This PR is only about dropping isort and black from pyproject.toml. Maybe we could discuss the special case mypy on discord? Whatever is decided, it should then be applied to poetry and poetry-core.

[finswimmer]

I think the way to do that would be to put language: system somewhere in your precommit configuration and then take responsibility yourself for running (eg) mypy in the correct environment.

@dimbleby: I guess you've something like this in your mind? https://jaredkhan.com/blog/mypy-pre-commit Looks like this is something we should give a try for mypy.

[dimbleby]

Yes, that's the sort of thing. I don't know how convenient this is though, you must need to do something to bootstrap the virtual environment eg install poetry and perhaps this is too heavyweight for a precommit hook.

To be honest I'm a little sceptical of the whole pre-commit approach. My inclination would be to do all the linting in a regular github workflow, just like the testing. Personally I wouldn't miss the git hook at all - though it's entirely reasonable to think the opposite.

(I don't think it's mypy that's the special case here. Most of the python linting tools expect to be in the project environment. If anything is the special case I'd suggest that it's black, which doesn't mind about the environment

Edit: apparently I am not keeping up with the isort world. It used to have lots of magic to decide sort order by detecting what was going on in the environment, but apparently as of its 5.0.0 release does not do this.)

[paulmelnikow]

Hiya, thought I would suggest a middle-of-the-road approach here, which is that you could put the linting dependencies into an extra, and then people can pick whether they want them or not. It's not quite as easy as installing by default, but it keeps the rest of the benefits.

[dimbleby]

The motivation is to avoid duplicating the list of dependencies between pyproject.toml and precommit config. I don't see how making them extras helps?

[paulmelnikow]

Ah, I see, they are already getting installed by pre-commit, not poetry.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[abn]

Fwiw, I agree with @finswimmer the goal is to make it easier to work with the code base and remove surprises when submitting pull requests. Additionally, this removes the unnecesary duplication for now. For black and isort, these can be fully managed by pre-commit and does not require to be in the project environment.

With that in mind, I say we merge this as this and deal with the mypy case as and when it pops up. This change is in line with what we do for poetry right now anyway.