Added formatting and precommit hook

[gubaidulinvadim]

Formatting tools and pre-commit hook identical to pyaml-cs-oa repository (made by Y. Hidaka). To use the recommit hooks one needs to

pip install pre-commit
pre-commit install
###
### To run manually the pre-commit hooks
pre-commit run --all-files

I've checked that the recommit hooks work correctly. I've also had to modify some parts of the code (typing) to pass mypy type checks. We would need to also add a run of ruff/mypy to the CI/CD pipeline to ensure that no one can commit code that is not formatted properly or does not pass the type checks.

The hooks and formatters are working but I need to fix a lot of mypy errors before the pull request review.

[JeanLucPons]

I'm not an expert with pre commit (we just run black in AT).
I get this issue.

[ubuntu20acu.pons] > pre-commit install
pre-commit installed at .git/hooks/pre-commit
[ubuntu20acu.pons] > pre-commit run --all-files
An error has occurred: InvalidConfigError: 
=====> .pre-commit-config.yaml is not a file
Check the log at /home/esrf/pons/.cache/pre-commit/pre-commit.log

[ubuntu20acu.pons] > git commit -m "Test 2" -a
No .pre-commit-config.yaml file was found
- To temporarily silence this, run `PRE_COMMIT_ALLOW_NO_CONFIG=1 git ...`
- To permanently silence this, install pre-commit with the --allow-missing-config option
- To uninstall pre-commit run `pre-commit uninstall`

[gubaidulinvadim]

I'm only getting this error if I try to do it on the main branch, where there's no .pre-commit-config.yaml file. On this branch, I get no error. This is a bit strange. @JeanLucPons you might need to reinstall pyaml with pip in dev mode.

pip install -e .[dev]
``

[JeanLucPons]

Thanks it worked. I definitely checked out to an other branch in between 2 tests !
Interesting results !

[ubuntu20acu.pons] > git commit -m "Test" pyaml/magnet/sextupole.py
...
pyaml/common/abstract.py:17: error: Missing return statement  [empty-body]
pyaml/common/abstract.py:17: note: If the method is meant to be abstract, use @abc.abstractmethod
pyaml/common/abstract.py:47: error: Missing return statement  [empty-body]
pyaml/common/abstract.py:47: note: If the method is meant to be abstract, use @abc.abstractmethod
pyaml/control/readback_value.py:35: error: Incompatible default for argument "timestamp" (default has type "None", argument has type "datetime")  [assignment]
pyaml/control/readback_value.py:35: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
pyaml/control/readback_value.py:35: note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase
pyaml/__init__.py:20: error: List item 1 has incompatible type "type[PyAMLException]"; expected "str"  [list-item]
pyaml/__init__.py:20: error: List item 2 has incompatible type "type[PyAMLConfigException]"; expected "str"  [list-item]
pyaml/magnet/magnet.py:23: error: Incompatible default for argument "model" (default has type "None", argument has type "MagnetModel")  [assignment]
pyaml/magnet/magnet.py:23: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
pyaml/magnet/magnet.py:23: note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase
pyaml/magnet/magnet.py:36: error: Incompatible types in assignment (expression has type "None", variable has type "ReadWriteFloatScalar")  [assignment]
pyaml/magnet/magnet.py:37: error: Incompatible types in assignment (expression has type "None", variable has type "ReadWriteFloatScalar")  [assignment]
pyaml/magnet/magnet.py:71: error: "Self" has no attribute "_cfg"  [attr-defined]
pyaml/magnet/sextupole.py:19: error: Argument 2 to "__init__" of "Magnet" has incompatible type "MagnetModel | None"; expected "MagnetModel"  [arg-type]
Found 10 errors in 5 files (checked 1 source file)

[gubaidulinvadim]

Overall, there are many mypy errors because the type hints are not done perfectly. It's normally easy to resolve, but it's routine work that takes some time.

[yhidaka]

I agree with @gubaidulinvadim. I think it's best to make formatting automated as soon as possible for cleaner diffs. From that point on, we never have to worry about this issue.

We should create a new PR to apply a one-time formatting on all existing files. We should coordinate with individual contributors who are actively working on open issues for the best timing to do this, or split this PR to multiple PRs so that this action can be delayed for some files if specifically asked by contributors.

My settings may well be sub-optimal. I was only using black/isort in pre-commit before. I switched to ruff/mypy as I saw in @gupichon 's repo, and ended up with the settings I used in an ad hoc manner. I also felt mypy seemed a bit aggressive, forcing type hints to be always correct and slowing me down. So, we could leave that out, if many hate it, but I feel it's better to be included.

[JeanLucPons]

I agree with @gubaidulinvadim. I think it's best to make formatting automated as soon as possible for cleaner diffs. From that point on, we never have to worry about this issue.

I also agree. ASAP.

[gubaidulinvadim]

@yhidaka , actually that this PR that aims to do that. I want to apply the formatting and fix mypy errors. Mypy can be configured to ignore some errors. But all type checkers are a bit aggressive like this.

[gubaidulinvadim]

I have commented out the mypy precommit hook. I would propose to go with the plan of @yhidaka and merge the ruff-formatting changes now, but merge mypy/type hints changes in a different merge request or as a part of several merge requests. This MR is ready for review of the formatting changes.

[yhidaka]

I have no experience with CI/CD, so cannot tell if the automated checking and blocking is also implemented. Is it done in this PR?

Also, wouldn't applying this now cause a problem to @gupichon, as he has lots of file changes in #78?

I'm sorry if these are stupid questions, as I'm not too familiar with all these collaborative workflows yet.

[gubaidulinvadim]

No it is not done. But it's a good point to add it to the CI/CD pipeline to prevent "improper" commits.

[gubaidulinvadim]

I've added CI/CD pipeline that should verify the ruff formatting. Unfortunately, I'm not using GitHub CI/CD as at SOLEIL we are only using GitLab. So, I'm not very familiar with slightly different syntax of GitHub workflows. I can also wait until the branch of Guillaume is merged and redo the formatting.

[JeanLucPons]

Thanks Vadim
It will help us
👍

[simoneliuzzo]

Dear @gubaidulinvadim,

could we add this in the "Contributing" tab of the main pyaml code page?

thank you
best regards
Simone