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

Use pre-commit as linter & Checker ✨ #345

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 9 additions & 28 deletions .github/workflows/run-on-pr.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,38 +9,19 @@ on:
- "doc/**"

jobs:
run-test:
name: ${{ matrix.python-version }}-${{ matrix.os }}-${{matrix.tox-env}}
runs-on: ${{ matrix.os }}
strategy:
# run this job using this matrix
matrix:
os:
- "ubuntu-latest"
python-version:
- "3.10"
tox-env:
- ""
- "-e pep8"
deploy:
runs-on: ubuntu-latest

fail-fast: false

# steps to run in each job. Some are github actions, others run shell commands
steps:
- name: Checkout repo
uses: actions/checkout@v2

- name: Set up python
- uses: actions/checkout@v2
- name: Set up Python
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python-version }}
architecture: ${{ matrix.architecture }}

python-version: "3.10"
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install --upgrade tox setuptools
pip list

- name: Run tests
run: tox ${{ matrix.tox-env }}
python -m pip install pre-commit
- name: Test Project linting
run: |
pre-commit run --all-files
Copy link
Member

Choose a reason for hiding this comment

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

this removes the test run, so maybe we can have this together with the current version

51 changes: 38 additions & 13 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -1,14 +1,39 @@
# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
repos:
- repo: https://github.com/python/black
rev: 21.9b0
hooks:
- id: black

- repo: https://github.com/sqlalchemyorg/zimports
rev: v0.4.1
hooks:
- id: zimports


- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v2.2.3
hooks:
- id: check-merge-conflict
- id: check-added-large-files
- id: check-ast
Copy link
Member

Choose a reason for hiding this comment

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

not sure it's really needed. we use test for this

- id: check-symlinks
- id: trailing-whitespace
- id: check-json
- id: debug-statements
- id: pretty-format-json
args: ["--autofix", "--allow-missing-credentials"]
- repo: https://github.com/PyCQA/isort
rev: 5.6.4
hooks:
- id: isort
args: ["--profile", "black"]
Copy link
Member

Choose a reason for hiding this comment

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

we use zimports so -1 here

- repo: https://gitlab.com/pycqa/flake8
rev: "8f9b4931b9a28896fb43edccb23016a7540f5b82"
hooks:
- id: flake8
additional_dependencies: [flake8-print]
files: '\.py$'
exclude: docs/
args:
- --select=F403,F406,F821,T003
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes more sense to keep the configuration in setup.cfg

Copy link
Member

Choose a reason for hiding this comment

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

also tox is probably a better place for this

Copy link
Member

Choose a reason for hiding this comment

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

+1 to @CaselIT's suggestion on this. I generally like the idea of linting/checking on a pre-commit hook, but those checks will often need to be run on-demand during development -- and configuring them within pre-commit would require them to be duplicated in other config files.

- repo: https://github.com/humitos/mirrors-autoflake
rev: v1.3
hooks:
- id: autoflake
files: '\.py$'
exclude: '^\..*'
args: ["--in-place"]
Copy link
Member

Choose a reason for hiding this comment

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

this seems to run autoflake. zimports does mostly the same

- repo: https://github.com/psf/black
rev: 19.10b0
hooks:
- id: black
args: ["--target-version", "py37"]
Copy link
Member

Choose a reason for hiding this comment

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

please keep the current version. also the setting should be in pyproject.toml

4 changes: 0 additions & 4 deletions pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
[build-system]
build-backend = 'setuptools.build_meta'
requires = ['setuptools >= 47', 'wheel']

[tool.black]
line-length = 79
target-version = ['py37']
17 changes: 0 additions & 17 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,3 @@ setenv=
cov: COVERAGE={[testenv]cov_args}

commands=pytest {env:COVERAGE:} {posargs}


[testenv:pep8]
basepython = python3
deps=
flake8
flake8-import-order
flake8-builtins
flake8-docstrings
flake8-rst-docstrings
pydocstyle<4.0.0
# used by flake8-rst-docstrings
pygments
black==21.9b0
commands =
flake8 ./mako/ ./test/ setup.py --exclude test/templates,test/foo {posargs}
black --check .