Add pre-commit as a development dependency

[scarlehoff]

Unless there is a strong reason why it should not be done, it is a development dependency.

[felixhekhorn]

Nope, we can't

• as said: poetry and pre-commit conflict with each other: your change would install pre-commit inside the poetry environment, but this would be (most of the time) useless, because it would force you to commit only from inside the environment. You would be forced to either spawn a shell (poetry shell) and commit from there or do something like poetry run git which both seems complicated
• and even if we would do so, pre-commit would still do it's own magic and create a separate virtual env (because it is written that way) - and at that point you have an virtual env inside an virtual env which is very dangerous. So I'd rather have them in parallel then nested ...
• note that also for this reason e.g. black is not a dev-dependecy, because it is managed by pre-commit

However, the abstract statement is true: pre-commit is required when working as a developer - it is simply not a dev-dependency in a poetry sense

[scarlehoff]

In what sense is it dangerous?

You are still allowed to have a global installation of pre-commit if you want but it allows you to have a self-contained dev environment with poetry.

It is not the same case as black, since it is managed by pre-commit ita version is pinned but the precommit version (if managed from the outside) it is not.

[felixhekhorn]

In what sense is it dangerous?

agreed, a better wording would be: "having an virtual env inside an virtual env can be very confusing" - of course one could do it, but how comfortable would that be?

You are still allowed to have a global installation of pre-commit if you want but it allows you to have a self-contained dev environment with poetry.

mmm? actually, no - if you want to have a self-contained dev env you could not have a global installation (that's the point right?) but that is not what would happen: pre-commit would always make it's own thing (take a look the installed hook)

It is not the same case as black, since it is managed by pre-commit ita version is pinned but the precommit version (if managed from the outside) it is not.

it is true that the pre-commit version is not pinned, but this is really a meta dependency at this point - meaning any change on pre-commit itself should have a very mild impact on the code (actually it should have none). If ever this may change the syntax of the .pre-commit-config.yaml. Instead the hooks and their versions can have a direct impact (e.g. black changed at some point it preferences and all powers changed).

Consider the purpose of the two tools: poetry is a replacement for setuptools, instead pre-commit is a git addon; poetry targets PyPI, pre-commit targets the repo; The fact that pre-commit is written in Python is completely accidental - and instead e.g. I'm using pre-commit in other contexts to run clang-format or prettier; it could have been written in perl.

[scarlehoff]

how comfortable would that be?

Right now I only have two sources of "stuff", either the global /usr or the project-local poetry environment. If I can't -INFN computers- (or don't want to) install in /usr the comfortable choice is the poetry environment just the same (I can do it manually of course regardless of this PR, I just thought it made sense to make it part of the development tools)

if you want to have a self-contained dev env you could not have a global installation

Exactly.

that is not what would happen: pre-commit would always make it's own thing

But I won't have to maintain what pre-commit does. I only need to care about where pre-commit is installed.

In any case, if you think having it as a dev dependencies will break use-cases for other people feel free to close the PR. I can simply manually install pre-commit whenever I clone the repo in a new computer.

[felixhekhorn]

I'll let @alecandido comment, but yes, I don't think this PR represents the common use case, so we should close it ...

(of course you're free to install pre-commit inside poetry nevertheless, but this then is your responsibility to solve the issues)

[scarlehoff]

I don't think this PR represents the common use case

With four developers this is the use case of the 25% of the users though :P

As I said, only if it doesn't break it for anyone. If it does break things then obviously I'll manage my own mess.
I don't see why would it be a problem: pre-commit stuff is still being saved in $HOME so it is not an env inside an env and, if pre-commit works as it should it can be any number of environments deep that it should be isolated.
Size-wise is an order of magnitude below ipython or sphinx and comparable with pdbpp which are all dev dependencies.

[felixhekhorn]

we can even do it ;-) but remember that installing pre-commit is not sufficient - you need to install the hooks!

and I still prefer to tell people "look you have to do this one-time install process and then you can do your usual git routines, including all the fancy add-ons for git and what not" than "you have to do this one-time install process and then every time you want to commit you have to activate the env"

[scarlehoff]

The second sentence is a tad shorter :P

You can still tell people to do the first though and leave the second for the connoisseur

[alecandido]

I didn't read the thread, I'll do, and we'll discuss.
I also have more abstracts and inspired arguments against adding pre-commit as a devdep, but the two most practical (and thus, arguably, strongest) ones are:

1. you want to be able to commit outside Poetry environment (or even without installing the project at all, e.g. if you are only modifying the README or something like) - pre-commit in any case is only for lightweight checks (no unit tests or similar - those in the workflows)
2. you don't want to have two different versions of pre-commit around
   in general you want one application to do something, and not several, possibly incompatible, similar ones to do the same things (making room for conflicts)

[scarlehoff]

if you are also against then lets leave it at that. More abstract arguments don’t really matter as they are also against! I’ll do my best to remember to install it when i need it.

but my counterarguments is that 1 forces you to have a second source of executables (which I don’t want to maintain in galileo or other infn computers) and to install precommit anyway (only globally).
and 2 is fixed by not installing a second one outside. Also, if it matters that the versions are different then we should truly make sure that everyone is using the same one, but we agreed it doesn’t.

[alecandido]

I'd be in favor of choosing the most sensible option, rather than going for majority.

I see your point about keeping it inside, in such way to make a reproducible environment: Poetry is there for that reason, and pre-commit is a development dependency.
In practice, we tried this way as the first shot, but it fell short. I also provided a script to install Poetry and pre-commit in order not to have the developer to install it every time (it was first in yadism, and then also in runcards as part of rr script), but it was a failure.

In order to use Poetry, you have to install it (and this is a separate source of executables, already). However, you can modify the repo w/o it (though it is generally not recommended, you should test while developing). Same goes for pre-commit: nice to have, and developer responsibility. But better to keep it simple and decoupled.

An alternative to global installation is make a "development environment" (e.g. with bare virtualenv) you live inside while developing, where you install both Poetry and pre-commit.
I do not recommend to stay inside poetry shell, nor for the editor, nor for running stuffs. Better to use poetry run <whatever-you-need>, including poetry run ipython (if you use the same long command over and over, you can consider to shorten it with a Poe script).

[scarlehoff]

I'd be in favor of choosing the most sensible option, rather than going for majority.

In this case is a qol change, doesn't really affect the project. If majority thinks it will introduce problems for their use cases I'm happy to concede.