Improve developer workflow with pre-commit (#351)

## Description  Pre-commit is a highly popular framework for installing pre-commit hooks into git projects. This PR adds pre-commit to improve quality-of-life for developers — with pre-commit, developers can easily enforce our style when contributing to pyribs. Adding pre-commit also necessitated fixing files which threw formatting and linting errors. ## TODO  - [x] Add pre-commit to CI - [x] Run YAPF on files and make changes - [x] Run prettier on Markdown/YAML files and make changes - [x] Update pylint config — previously, we threw many warnings when running the latest pylint because our pylint config included features only intended for Python 2 compatibility - [x] Allow `__init__` methods to not have docstrings - [x] Install pytest in the CI so the tests can import it during the pylint check ## Questions  ## Status - [x] I have read the guidelines in [CONTRIBUTING.md](https://github.com/icaros-usc/pyribs/blob/master/CONTRIBUTING.md) - [x] I have formatted my code using `yapf` - [x] I have tested my code by running `pytest` - [x] I have linted my code with `pylint` - [x] I have added a one-line description of my change to the changelog in `HISTORY.md` - [x] This PR is ready to go
icaros-usc · Sep 4, 2023 · 8f5b8c0 · 8f5b8c0
1 parent daeae2e
commit 8f5b8c0
Show file tree

Hide file tree

Showing 18 changed files with 128 additions and 152 deletions.
diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md
@@ -16,9 +16,11 @@
 
 ## Status
 
-- [ ] I have read the guidelines in [CONTRIBUTING.md](https://github.com/icaros-usc/pyribs/blob/master/CONTRIBUTING.md)
+- [ ] I have read the guidelines in
+      [CONTRIBUTING.md](https://github.com/icaros-usc/pyribs/blob/master/CONTRIBUTING.md)
 - [ ] I have formatted my code using `yapf`
 - [ ] I have tested my code by running `pytest`
 - [ ] I have linted my code with `pylint`
-- [ ] I have added a one-line description of my change to the changelog in `HISTORY.md`
+- [ ] I have added a one-line description of my change to the changelog in
+      `HISTORY.md`
 - [ ] This PR is ready to go
diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml
@@ -11,6 +11,20 @@ on:
       - master
 
 jobs:
+  pre-commit:
+    runs-on: ubuntu-latest
+    env:
+      # Skipped for now since we just added pre-commit and not all of our code
+      # perfectly passes pylint.
+      SKIP: pylint
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions/setup-python@v3
+        with:
+          python-version: 3.11
+      - name: Install all deps and pylint (to be available to pre-commit)
+        run: pip install .[all] pylint
+      - uses: pre-commit/[email protected]
   # The visualize extra is only tested with pinned reqs because different
   # Matplotlib versions have slightly different outputs.
   test:
@@ -133,7 +147,7 @@ jobs:
         run: make docs
   deploy:
     runs-on: ubuntu-latest
-    needs: [test, pin, coverage, benchmarks, examples, tutorials]
+    needs: [pre-commit, test, pin, coverage, benchmarks, examples, tutorials]
     if: startsWith(github.ref, 'refs/tags')
     steps:
       - uses: actions/checkout@v3

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
@@ -0,0 +1,40 @@
+exclude: LICENSE
+repos:
+  - repo: https://github.com/pre-commit/pre-commit-hooks
+    rev: v4.4.0
+    hooks:
+      # See https://pre-commit.com/hooks.html
+      - id: check-added-large-files
+      - id: check-symlinks
+      - id: check-yaml
+      - id: debug-statements
+      - id: end-of-file-fixer
+      - id: mixed-line-ending
+      - id: trailing-whitespace
+  - repo: https://github.com/google/yapf
+    rev: v0.33.0
+    hooks:
+      - id: yapf
+  - repo: https://github.com/pycqa/isort
+    rev: 5.11.5
+    hooks:
+      - id: isort
+        name: isort (python)
+  - repo: https://github.com/pre-commit/mirrors-prettier
+    rev: v3.0.2
+    hooks:
+      - id: prettier
+        types_or: [markdown, yaml]
+  # pylint runs locally due to importing modules. See
+  # https://pylint.pycqa.org/en/latest/user_guide/installation/pre-commit-integration.html
+  - repo: local
+    hooks:
+      - id: pylint
+        name: pylint
+        entry: pylint
+        language: system
+        types: [python]
+        args: [
+            "-rn", # Only display messages
+            "-sn", # Don't display the score
+          ]
diff --git a/.pylintrc b/.pylintrc
@@ -34,15 +34,6 @@ unsafe-load-any-extension=no
 # run arbitrary code
 extension-pkg-whitelist=numpy
 
-# Allow optimization of some AST trees. This will activate a peephole AST
-# optimizer, which will apply various small optimizations. For instance, it can
-# be used to obtain the result of joining multiple strings with the addition
-# operator. Joining a lot of strings can lead to a maximum recursion error in
-# Pylint and this flag can prevent that. It has one side effect, the resulting
-# AST will be different than the one from reality. This option is deprecated
-# and it will be removed in Pylint 2.0.
-optimize-ast=no
-
 
 [MESSAGES CONTROL]
 
@@ -65,7 +56,7 @@ confidence=
 # --enable=similarities". If you want to run only the classes checker, but have
 # no Warning level messages displayed, use"--disable=all --enable=classes
 # --disable=W"
-disable=long-suffix,standarderror-builtin,indexing-exception,delslice-method,unichr-builtin,dict-view-method,parameter-unpacking,unicode-builtin,cmp-builtin,intern-builtin,round-builtin,backtick,nonzero-method,xrange-builtin,coerce-method,raw_input-builtin,old-division,filter-builtin-not-iterating,old-octal-literal,input-builtin,map-builtin-not-iterating,buffer-builtin,basestring-builtin,zip-builtin-not-iterating,using-cmp-argument,unpacking-in-except,old-raise-syntax,coerce-builtin,dict-iter-method,hex-method,range-builtin-not-iterating,useless-suppression,cmp-method,print-statement,reduce-builtin,file-builtin,long-builtin,getslice-method,execfile-builtin,no-absolute-import,metaclass-assignment,oct-method,reload-builtin,import-star-module-level,suppressed-message,apply-builtin,raising-string,next-method-called,setslice-method,old-ne-operator,arguments-differ,wildcard-import,locally-disabled
+disable=suppressed-message,arguments-differ,wildcard-import,locally-disabled,duplicate-code
 
 
 [REPORTS]
@@ -75,12 +66,6 @@ disable=long-suffix,standarderror-builtin,indexing-exception,delslice-method,uni
 # mypackage.mymodule.MyReporterClass.
 output-format=text
 
-# Put messages in a separate file for each module / package specified on the
-# command line instead of printing them on stdout. Reports (if any) will be
-# written in a file name "pylint_global.[txt|html]". This option is deprecated
-# and it will be removed in Pylint 2.0.
-files-output=no
-
 # Tells whether to display a full report or only the messages
 reports=yes
 
@@ -108,76 +93,43 @@ bad-names=foo,bar,baz,toto,tutu,tata
 # the name regexes allow several styles.
 name-group=
 
-# Include a hint for the correct naming format with invalid-name
-include-naming-hint=no
-
 # List of decorators that produce properties, such as abc.abstractproperty. Add
 # to this list to register other decorators that produce valid properties.
 property-classes=abc.abstractproperty
 
 # Regular expression matching correct variable names
 variable-rgx=[a-z_][a-z0-9_]{0,30}$
 
-# Naming hint for variable names
-variable-name-hint=[a-z_][a-z0-9_]{0,30}$
-
 # Regular expression matching correct class attribute names
 class-attribute-rgx=([A-Za-z_][A-Za-z0-9_]{0,30}|(__.*__))$
 
-# Naming hint for class attribute names
-class-attribute-name-hint=([A-Za-z_][A-Za-z0-9_]{0,30}|(__.*__))$
-
 # Regular expression matching correct argument names
 argument-rgx=[a-z_][a-z0-9_]{0,30}$
 
-# Naming hint for argument names
-argument-name-hint=[a-z_][a-z0-9_]{0,30}$
-
 # Regular expression matching correct module names
 module-rgx=(([a-z_][a-z0-9_]*)|([A-Z][a-zA-Z0-9]+))$
 
-# Naming hint for module names
-module-name-hint=(([a-z_][a-z0-9_]*)|([A-Z][a-zA-Z0-9]+))$
-
 # Regular expression matching correct constant names
 const-rgx=(([A-Z_][A-Z0-9_]*)|(__.*__))$
 
-# Naming hint for constant names
-const-name-hint=(([A-Z_][A-Z0-9_]*)|(__.*__))$
-
 # Regular expression matching correct inline iteration names
 inlinevar-rgx=[A-Za-z_][A-Za-z0-9_]*$
 
-# Naming hint for inline iteration names
-inlinevar-name-hint=[A-Za-z_][A-Za-z0-9_]*$
-
 # Regular expression matching correct method names
 method-rgx=[a-z_][a-z0-9_]{0,30}$
 
-# Naming hint for method names
-method-name-hint=[a-z_][a-z0-9_]{0,30}$
-
 # Regular expression matching correct function names
 function-rgx=[a-z_][a-z0-9_]{0,50}$
 
-# Naming hint for function names
-function-name-hint=[a-z_][a-z0-9_]{0,50}$
-
 # Regular expression matching correct attribute names
 attr-rgx=[a-z_][a-z0-9_]{0,30}$
 
-# Naming hint for attribute names
-attr-name-hint=[a-z_][a-z0-9_]{0,30}$
-
 # Regular expression matching correct class names
 class-rgx=[A-Z_][a-zA-Z0-9]+$
 
-# Naming hint for class names
-class-name-hint=[A-Z_][a-zA-Z0-9]+$
-
 # Regular expression which should only match function or class names that do
 # not require a docstring.
-no-docstring-rgx=^(test|benchmark)_
+no-docstring-rgx=^(test|benchmark)_|__init__
 
 # Minimum line length for functions/classes that require docstrings, shorter
 # ones are exempt.
@@ -203,12 +155,6 @@ ignore-long-lines=(^\s*(# )?<?https?://\S+>?$)|(^\s*<.*>`_.?$)
 # else.
 single-line-if-stmt=y
 
-# List of optional constructs for which whitespace checking is disabled. `dict-
-# separator` is used to allow tabulation in dicts, etc.: {1  : 1,\n222: 2}.
-# `trailing-comma` allows a space between comma and closing bracket: (a, ).
-# `empty-line` allows space-only lines.
-no-space-check=trailing-comma,dict-separator
-
 # Maximum number of lines in a module
 max-module-lines=2000
 
@@ -405,4 +351,4 @@ analyse-fallback-blocks=no
 
 # Exceptions that will emit a warning when being caught. Defaults to
 # "Exception"
-overgeneral-exceptions=Exception
+overgeneral-exceptions=builtins.Exception
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -1,30 +1,11 @@
 # Contributing
 
-Contributions are welcome, and they are greatly appreciated. Every little bit
-helps, and credit will always be given.
-
-## Types of Contributions
-
-- **Report Bugs:** Refer to the
-  [Issue Tracker](https://github.com/icaros-usc/pyribs/issues). Please include
-  details such as operating system, Python version, and ribs version, as well as
-  detailed steps to reproduce the bug.
-- **Fix Bugs:** Look through the Issue Tracker for bugs. Anything tagged with
-  "bug" and "help wanted" is open to whoever wants to implement it.
-- **Propose features:** To request new features in pyribs, submit a Feature
-  Request on the Issue Tracker. In the request, please:
-  - Explain in detail how the feature would work.
-  - Keep the scope as narrow as possible, to make the features easier to
-    implement.
-- **Implement Features:** Look through the Issue Tracker for features. Anything
-  tagged with "enhancement" and "help wanted" is open to whoever wants to
-  implement it.
-- **Write Documentation:** pyribs could always use more documentation, whether
-  as part of the official pyribs docs, in docstrings, or even on the web in blog
-  posts, articles, and such. For the website, refer to the
-  [website repo](https://github.com/icaros-usc/pyribs.org).
-- **Submit Feedback:** The best way to send feedback is to file an issue on the
-  [Issue Tracker](https://github.com/icaros-usc/pyribs/issues).
+Contributions are welcome and are greatly appreciated! Every little bit helps.
+Contributions include reporting/fixing bugs, proposing/implementing features
+(see our [Issue Tracker](https://github.com/icaros-usc/pyribs/issues)), writing
+documentation in the codebase or on our
+[website repo](https://github.com/icaros-usc/pyribs.org), and submitting
+feedback.
 
 ## Developing pyribs
 
@@ -41,7 +22,13 @@ Ready to contribute? Here's how to set up pyribs for local development.
    git clone https://github.com/USERNAME/pyribs.git
    ```
 
-1. Install the local copy and dev requirements into an environment. For
+1. Create a branch for local development:
+
+   ```bash
+   git checkout -b name-of-bugfix-or-feature
+   ```
+
+1. Install the local copy and dev requirements into a virtual environment. For
    instance, with Conda, the following creates an environment at `./env`.
 
    ```bash
@@ -51,24 +38,30 @@ Ready to contribute? Here's how to set up pyribs for local development.
    pip install -e .[dev]
    ```
 
-1. Create a branch for local development:
+1. We roughly follow the
+   [Google Style Guide](https://google.github.io/styleguide/pyguide.html) in our
+   codebase by using yapf, isort, and pylint to enforce code format and style.
+   To automatically check for formatting and style every time you commit, we use
+   [pre-commit](https://pre-commit.com). Pre-commit should have already been
+   installed with `.[dev]` above. To set it up, run:
 
    ```bash
-   git checkout -b name-of-bugfix-or-feature
+   pre-commit install
    ```
 
-   Now make the appropriate changes locally.
+1. Now make the appropriate changes locally. If relevant, make sure to write
+   tests for your code in the `tests/` folder.
 
-   - Please follow the
-     [Google Style Guide](https://google.github.io/styleguide/pyguide.html)
-     (particularly when writing docstrings).
-   - Make sure to auto-format the code using YAPF. We highly recommend
-     installing an editor plugin that auto-formats on save, but YAPF also runs
-     on the command line:
+1. Auto-format and lint your code using YAPF, isort, and pylint. Note that
+   pre-commit will automatically run these whenever you commit your code; you
+   can also run them with `pre-commit run`. You can also run these commands on
+   the command line:
 
-     ```bash
-     yapf -i FILES
-     ```
+   ```bash
+   yapf -i FILES
+   isort FILES
+   pylint FILES
+   ```
 
 1. After making changes, check that the changes pass the tests:
 
@@ -84,16 +77,6 @@ Ready to contribute? Here's how to set up pyribs for local development.
    make benchmark # ^ same as above
    ```
 
-   Finally, to lint the code:
-
-   ```bash
-   pylint ribs tests benchmarks examples
-   make lint # ^ same as above
-   ```
-
-   To get pytest and pylint, pip install them into the environment. However,
-   they should already install with `pip install -e .[dev]`.
-
 1. Add your change to the changelog for the current version in `HISTORY.md`.
 
 1. Commit the changes and push the branch to GitHub:
@@ -104,21 +87,7 @@ Ready to contribute? Here's how to set up pyribs for local development.
    git push origin name-of-bugfix-or-feature
    ```
 
-1. Submit a pull request through the GitHub website.
-
-## Pull Request Guidelines
-
-Before submitting a pull request, check that it meets these guidelines:
-
-1. Style: Code should follow the
-   [Google Style Guide](https://google.github.io/styleguide/pyguide.html) and be
-   auto-formatted with [YAPF](https://github.com/google/yapf).
-1. The pull request should include tests.
-1. If the pull request adds functionality, corresponding docstrings and other
-   documentation should be updated.
-1. The pull request should work for Python 3.8 and higher. GitHub Actions will
-   display test results at the bottom of the pull request page. Check there for
-   test results.
+1. Submit a pull request through the GitHub web interface.
 
 ## Instructions
 
@@ -209,8 +178,7 @@ their source is shown in the docs. To create an example:
 ### Referencing Papers
 
 When referencing papers, refer to them as `Lastname YEAR`, e.g. `Smith 2004`.
-Also, prefer to link to the paper's website, rather than just the PDF. This is
-particularly relevant when linking to arXiv papers.
+Also, prefer to link to the paper's website, rather than just the PDF.
 
 ### Deploying
 

diff --git a/HISTORY.md b/HISTORY.md
@@ -13,6 +13,10 @@
 - Use dask instead of multiprocessing for lunar lander tutorial (#346)
 - pip install swig before gymnasium[box2d] in lunar lander tutorial (#346)
 
+#### Improvements
+
+- Improve developer workflow with pre-commit (#351)
+
 ## 0.5.2
 
 This release contains miscellaneous edits to our documentation from v0.5.1.

diff --git a/benchmarks/cvt_add.py b/benchmarks/cvt_add.py
@@ -89,14 +89,15 @@ def main():
     # Set up these archives so we can use the same centroids across all
     # experiments for a certain number of cells (and also save time).
     ref_archives = {
-        cells: CVTArchive(
-            solution_dim=all_solution_batch.shape[2],
-            cells=cells,
-            ranges=[(-1, 1), (-1, 1)],
-            # Use 200k cells to avoid dropping clusters. However, note that we
-            # no longer test with 10k cells.
-            samples=100_000 if cells != 10_000 else 200_000,
-            use_kd_tree=False) for cells in n_cells
+        cells:
+            CVTArchive(
+                solution_dim=all_solution_batch.shape[2],
+                cells=cells,
+                ranges=[(-1, 1), (-1, 1)],
+                # Use 200k cells to avoid dropping clusters. However, note that we
+                # no longer test with 10k cells.
+                samples=100_000 if cells != 10_000 else 200_000,
+                use_kd_tree=False) for cells in n_cells
     }
 
     def setup(cells, use_kd_tree):