Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pre-commit setup for the repo #1071

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

maichmueller
Copy link
Contributor

@maichmueller maichmueller commented May 19, 2023

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.

@lanctot
Copy link
Collaborator

lanctot commented May 24, 2023

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
Copy link
Collaborator

lanctot commented Oct 23, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants