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.

Sign up for GitHub

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

Jump to bottom

Add precommit #2790

Merged
merged 7 commits into from
Dec 15, 2021
Merged

Add precommit #2790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
23baf9c
Add precommit config
SauravMaheshkar Dec 11, 2021
17a4565
Add pre-commit to environments
SauravMaheshkar Dec 11, 2021
5ea8484
change yapf version to v0.22.0
Dec 14, 2021
996d526
Remove pytest from pre-commit
Dec 14, 2021
51534d3
remove pre-commit from env files apart from common
SauravMaheshkar Dec 14, 2021
a2424d9
Add documentation for pre-commit
SauravMaheshkar Dec 14, 2021
1f43aa0
Update pre-commit documentation
SauravMaheshkar Dec 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v3.4.0
hooks:
- id: check-merge-conflict
- id: trailing-whitespace
- id: end-of-file-fixer
- id: check-yaml

- repo: https://github.com/pre-commit/mirrors-yapf
rev: v0.22.0
hooks:
- id: yapf

- repo: https://gitlab.com/PyCQA/flake8
rev: 3.8.4
hooks:
- id: flake8
args: [--count]

- repo: https://github.com/pre-commit/mirrors-mypy
arunppsg marked this conversation as resolved.
Show resolved Hide resolved
rev: v0.790
hooks:
- id: mypy
args: [--ignore-missing-imports]
21 changes: 21 additions & 0 deletions docs/source/development_guide/coding.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,27 @@
Coding Conventions
==================

Pre-Commit
-----------

.. _`Pre-Commit`: https://pre-commit.com/

We use `pre-commit`_ to ensure that we're always keeping up with the best
practices when it comes to linting, standard code conventions and type
annotations. Although it may seem time consuming at first as to why is one
supposed to run all these tests and checks but it helps in identifying simple
issues before submission to code review. We've already specified a configuration
file with a list of hooks that will get executed before every commit.

First you'll need to setup the git hook scripts by installing them.

.. code-block:: bash

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to do pip install pre-commit first right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also try these instructions out locally on a fresh environment/deepchem install to make sure they work as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pre-commit is already present in env.common.yml so there shouldn't be any need to run pip install pre-commit right ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure actually. Could you try installing from source in a fresh environment following directions in the doc and see if it is present for you? The installation flow is a little complex so I'm not sure if the user will actually have pre-commit installed correctly on a fresh install

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's present by default. Steps I followed:

  • Clone my branch (add-precommit) to my local system and create a new conda env named test using env.common.yml.
  • activate the test environment and run pre-commit install. PFA the output
pre-commit installed at .git/hooks/pre-commit
  • run pre-commit run --all-files this runs pre-commit on the codebase. Output is as follows :
[INFO] Installing environment for https://github.com/pre-commit/mirrors-yapf.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
Check for merge conflicts................................................Passed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: There seems to be a lot of yapf and flake8 inconsistencies in the codebase.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch! If you're interested, a follow-up PR cleaning up yapf/flake8 inconsistencies in the codebase would be very useful

pre-commit install

Now whenever you commit, pre-commit will run the necessary hooks on the modified
files.

Code Formatting
---------------

Expand Down
1 change: 1 addition & 0 deletions env.common.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ dependencies:
- rdkit
- mdtraj
- numpy==1.19.*
- pre-commit
- pip==20.2.*
- pip:
- biopython
Expand Down