open_spiel icon indicating copy to clipboard operation
open_spiel copied to clipboard

Pre-commit setup for the repo

Open maichmueller opened this issue 2 years ago • 2 comments

This PR addresses what has been mentioned in #1070.

What this PR includes:

  • pre-commit configuration for formatters:
    • clang-format: c++ project files formatting
    • pyink: google's python formatter that is close to black, but allows 2-indent formatting (oh the humanity...)
    • cmake-format: formatting style for cmake files
    • isort: sort python imports alphabetically (can use profile=black, so I hope this profile works with pyink too)
    • black: the python formatter (commented out but added for when google policy on 2-indent changes)
  • pylint: using google's official configuration. Since this config enforces 2-indent, pyink is needed.
  • common pre-commit checks:
    • id: check-added-large-files
    • id: check-case-conflict
    • id: check-docstring-first (currently uncommented as some files would need manual changing)
    • id: check-merge-conflict
    • id: check-symlinks
    • id: check-toml
    • id: check-yaml
    • id: debug-statements
    • id: end-of-file-fixer
    • id: mixed-line-ending
    • id: requirements-txt-fixer
    • id: trailing-whitespace
  • common python checks:
    • python-check-blanket-noqa
    • id: python-check-blanket-type-ignore
    • id: python-no-log-warn (uncommented because clashes in multiple places where logging.warn has been used)
    • id: python-no-eval
    • id: python-use-type-annotations

As a side effect of the pre-commit checks I have moved the setup definition into a pyproject.toml, since it allows to also write the configurations for some of these tools. In consequence:

  • pyproject.toml now holds the metadata of the project (version, authors, license, etc.)
  • setup.py is only the extension builder

The dev-tools needed to run these pre-commits have also been added to the the pyproject.toml as optional dependencies of the dev group and could then be installed via:

pip install 'open_spiel[dev]'

or locally

pip install '.[dev]'

If this PR is accepted a remark in the docs should be made for developers who wish to contribute the upstream to run pre-commit install on their repo in order to activate the hooks.

maichmueller avatar May 19 '23 10:05 maichmueller

Thanks @maichmueller !

Just a heads up: it will take a bit of time before we get to this as it involves learning a few things and discussion with the team.

Also we have a release coming up that I would like to get out first (hopefully next week).

lanctot avatar May 24 '23 19:05 lanctot

We might import this (I think either way we will have to move to using pyproject.toml eventually) so I will look further into it eventually.

Could you pull from master and push the merge commit to resolve the branch conflicts?

lanctot avatar Oct 23 '23 10:10 lanctot