refactor: move ruff and black from the format file to the pre-commit hooks

[12rambau]

Description

I ask myself, why using a format script to lint the lib instead of a pre-commit hook ? Looking at the format file, it's only using ruff and black so as they have very nicely design hooks I used them instead.

Checklist

• Add test cases to all the changes you introduce
• Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
• Test the changes on the local machine manually
• Update the documentation for the changes

If you agree with this modification, I'll update the PR/documentation instructions

related to #698

[woile]

I'm a heavy user of the format script, so please leave it. And also leave them as dev dependencies on the pyproject.toml, they are used by the CI and the IDE's.

Besides that, I think it's a good idea to have them as well in the pre-commit

[12rambau]

thanks @woile for the feedback, let me advocate a bit for this proposed change before I remove them:

I'm a heavy user of the format script, so please leave it.

if they are part of the pre-commits what is preventing you from executing them all at once in 5s with pre-commit run -a while developping ? This command is widelly known so no need for specific explaination to contributors. Just say "run the pre-commits".

also leave them as dev dependencies on the pyproject.toml, they are used by the CI and the IDE's.

If they are used in actions, why not using the pre-commit actions https://github.com/12rambau/sepal_ui/blob/cbca0af9a09e347261af068a40be06ca93b62c5c/.github/workflows/unit.yml#L15

The advantage that I see are:

• only 1 version for everything (local, CI, hooks)
• no need for linting deps in the lib
• can be used in any CI by running pre-commit run -a or the nicer github action

the issue for me with the current formatting:

• multiplication of piloting files: format, pre-commit-hook, github actions
• duplication of dependencies e.g. black/ruff is tested in both format and pre-commit hooks which use 2 different environments
• On my end I deactivated pre-commits because of installation issues for format and test which never occurs to me with github based hooks. (I assume it was due to my poetry version)

If you are really against it, there is no point to my PR as the main objective is to use one and only config for linting/format operation and make it centralized in the pre-commits-config file. Let me know and feel free to close it (along with the associated issue).

[noirbizarre]

To be honest, I hesitated to submit the same kind of change, use ruff and black pre-commit plugins like this PR, but also:

• use commitizen pre-commit plugin (dogfooding, if it doesn't work for commitizen itself, this is an issue)
• use mypy pre-commit plugin
• target a single Python version for linting (targeting all version with MyPy result in a false positive and some ugly work-around to support both 3.7 and 3.11)
• remove the tests from pre-commit (time lost when you already spent some time on your PR running tests again and again)
• remove pre-commit on push (redundant because you push commits that already pass the test)
• remove the scripts/*
• use a script runner based on pyproject.toml for dev scripts (like https://poethepoet.natn.io/)
• add a tox.ini to test against all supported versions

Note that in the current state, sometimes you need to perform a poetry install before committing or pushing because entrypoints needs to be up to date.

In fact I already have a branch with some of those changes (pre-commit changes) and I have a local tox.ini file for testing.
Adding Poe The Poet wouldn't cost much to add to the PR.

So I am really interested in this discussion and I would love to contribute my changes too.

[12rambau]

@noirbizarre I agree with your analysis but packaging modification have lots of impact on existing branches and forks so in my experience it's better to work with baby steps like this one instead of 1 single PR that will lead to huge conflicts for everyone else. If you look at the associated issue, it's more or less in the pipe. Maybe we can discuss there with the maintainers and keep things simple in this PR ?

[Lee-W]

Hi @12rambau @noirbizarre , sorry for the late reply. I just read through this discussion. I like @noirbizarre 's detailed idea and agree with @12rambau that we should take baby steps. Here are some of my thoughts regarding each point.

target a single Python version for linting (targeting all version with MyPy result in a false positive and some ugly work-around to support both 3.7 and 3.11)

Python 3.7 is about to reach its EOL and I think it would still be better if we could lint on each version. It still finds some unexpected errors earlier. This strategy is also applied to many large projects as well.

remove the tests from pre-commit (time lost when you already spent some time on your PR running tests again and again)

I'm not sure. I still wish everyone actually ran the test once before they pushed. I'm ok with moving it to push only

remove pre-commit on push (redundant because you push commits that already pass the test)

The reason of using pre-commit on push is to run the long-running tasks that don't need to be run everytime.

use a script runner based on pyproject.toml for dev scripts (like https://poethepoet.natn.io/)

This is awesome 🤩

add a tox.ini to test against all supported versions

Just a random idea, should we try nox? But tox is good for me as well

[Lee-W]

@woile Would love to hear your thoughts 🙂

[12rambau]

Just a random idea, should we try nox? But tox is good for me as well

I am a nox person so I would prefer to use it instead of tox but that 100% personnal though functionalities are more or less the same.

As mentioned earlier in the PR, I think baby steps will be safer so maybe @noirbizarre comments could be moved to an issue and we could iterate in other PRs.

[12rambau]

I now face conflicts in most of the files (nothing severe though), anything I should do to move this PR forward ?

[Lee-W]

I think we'll need @woile 's feedback on this one