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 Hooks (black, flake8, mypy, etc) [CT-105] #4639

Merged
merged 34 commits into from
Feb 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
fb9cca1
testing pre-commit hook
iknox-fa Jan 27, 2022
e9ac78e
test pre-commit 2
iknox-fa Jan 27, 2022
3f48ea4
adding pre-commit hook
iknox-fa Jan 27, 2022
45db18b
return setup.py
iknox-fa Jan 27, 2022
9fafac6
return setup.py again
iknox-fa Jan 27, 2022
75201be
PR feedback
iknox-fa Feb 1, 2022
a7d99a9
PR feedback
iknox-fa Feb 1, 2022
4d44780
adjust tox settings for flake8 to match black
iknox-fa Feb 3, 2022
675d551
adding flake8 to hooks
iknox-fa Feb 4, 2022
35a6447
updates.. moar
iknox-fa Feb 4, 2022
d3865d7
Makefile changes
iknox-fa Feb 4, 2022
3d65519
Makefile Makeove! Squeee
iknox-fa Feb 4, 2022
181dd12
fixed gitignore bug + tidying
iknox-fa Feb 4, 2022
e66aa6a
removed tox lint targets
iknox-fa Feb 4, 2022
f11b323
merge main
iknox-fa Feb 4, 2022
bce4bb1
Regexes.. Ugh
iknox-fa Feb 4, 2022
c29e7ee
nope
iknox-fa Feb 4, 2022
7e1af5d
swapped pre-commit in for tox on code checks
iknox-fa Feb 4, 2022
8d900e9
fixups for mypy and test exclusion
iknox-fa Feb 8, 2022
4f35cbf
PR feedback
iknox-fa Feb 8, 2022
cb09add
PR feedback
iknox-fa Feb 8, 2022
64eede8
PR feedback
iknox-fa Feb 8, 2022
b7a6d01
PR feedback
iknox-fa Feb 8, 2022
522b4f7
PR feedback
iknox-fa Feb 8, 2022
7c13388
PR feedback
iknox-fa Feb 8, 2022
a18c790
PR Feedback
iknox-fa Feb 8, 2022
8477fdb
PR feedback
iknox-fa Feb 8, 2022
33727ed
updated makefile, added pre-commit targets
iknox-fa Feb 9, 2022
2161a53
updated stages for checks
iknox-fa Feb 10, 2022
8af83a0
PR feedback
iknox-fa Feb 10, 2022
2df7667
removed black from make lint (too slow)
iknox-fa Feb 11, 2022
e5cf9fa
merge master
iknox-fa Feb 11, 2022
e2f7f5a
updating workflow to install mypy
iknox-fa Feb 11, 2022
e64cc0a
updating workflow to install editable reqs
iknox-fa Feb 11, 2022
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
12 changes: 12 additions & 0 deletions .flake8
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[flake8]
select =
E
W
F
ignore =
W503 # makes Flake8 work like black
W504
E203 # makes Flake8 work like black
E741
max-line-length = 99
exclude = test
23 changes: 9 additions & 14 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,10 @@ defaults:

jobs:
code-quality:
name: ${{ matrix.toxenv }}
name: code-quality

runs-on: ubuntu-latest

strategy:
fail-fast: false
matrix:
toxenv: [flake8, mypy]

env:
TOXENV: ${{ matrix.toxenv }}
PYTEST_ADDOPTS: "-v --color=yes"

steps:
- name: Check out the repository
uses: actions/checkout@v2
Expand All @@ -62,12 +53,16 @@ jobs:
- name: Install python dependencies
run: |
pip install --user --upgrade pip
pip install tox
pip install pre-commit
pip install mypy==0.782
pip install -r editable-requirements.txt
pip --version
tox --version
pre-commit --version
mypy --version
dbt --version

- name: Run tox
run: tox
- name: Run pre-commit hooks
run: pre-commit run --all-files --show-diff-on-failure

unit:
name: unit test / python ${{ matrix.python-version }}
Expand Down
1 change: 0 additions & 1 deletion .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ jobs:
run: |
dbt --version


github-release:
name: GitHub Release

Expand Down
9 changes: 5 additions & 4 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@ coverage.xml
*,cover
.hypothesis/
test.env
*.pytest_cache/

# Mypy
.mypy_cache/
Comment on lines -53 to -54
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did this go? do we need the new location in the gitignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a duplicate. See line 27


# Translations
*.mo
Expand All @@ -66,10 +65,10 @@ docs/_build/
# PyBuilder
target/

#Ipython Notebook
# Ipython Notebook
.ipynb_checkpoints

#Emacs
# Emacs
*~

# Sublime Text
Expand All @@ -78,6 +77,7 @@ target/
# Vim
*.sw*

# Pyenv
.python-version

# Vim
Expand All @@ -90,6 +90,7 @@ venv/
# AWS credentials
.aws/

# MacOS
.DS_Store

# vscode
Expand Down
66 changes: 66 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
# Configuration for pre-commit hooks (see https://pre-commit.com/).
# Eventually the hooks described here will be run as tests before merging each PR.

# TODO: remove global exclusion of tests when testing overhaul is complete
exclude: ^test/
iknox-fa marked this conversation as resolved.
Show resolved Hide resolved

# Force all unspecified python hooks to run python 3.8
default_language_version:
python: python3.8

repos:
iknox-fa marked this conversation as resolved.
Show resolved Hide resolved
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v3.2.0
hooks:
- id: check-yaml
args: [--unsafe]
- id: check-json
- id: end-of-file-fixer
- id: trailing-whitespace
- id: check-case-conflict
- repo: https://github.com/psf/black
rev: 21.12b0
hooks:
- id: black
args:
- "--line-length=99"
- "--target-version=py38"
- id: black
alias: black-check
stages: [manual]
args:
- "--line-length=99"
- "--target-version=py38"
- "--check"
- "--diff"
- repo: https://gitlab.com/pycqa/flake8
rev: 4.0.1
hooks:
- id: flake8
- id: flake8
alias: flake8-check
stages: [manual]
- repo: https://github.com/pre-commit/mirrors-mypy
rev: v0.782
hooks:
- id: mypy
# N.B.: Mypy is... a bit fragile.
#
# By using `language: system` we run this hook in the local
# environment instead of a pre-commit isolated one. This is needed
# to ensure mypy correctly parses the project.

# It may cause trouble
# in that it adds environmental variables out of our control to the
# mix. Unfortunately, there's nothing we can do about per pre-commit's
# author.
# See https://github.com/pre-commit/pre-commit/issues/730 for details.
args: [--show-error-codes]
files: ^core/dbt/
language: system
- id: mypy
alias: mypy-check
stages: [manual]
args: [--show-error-codes, --pretty]
files: ^core/dbt/
language: system
13 changes: 7 additions & 6 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ Here's a good workflow:
- Outline your planned implementation. If you want help getting started, ask!
- Follow the steps outlined below to develop locally. Once you have opened a PR, one of the `dbt-core` maintainers will work with you to review your code.
- Add a test! Tests are crucial for both fixes and new features alike. We want to make sure that code works as intended, and that it avoids any bugs previously encountered. Currently, the best resource for understanding `dbt-core`'s [unit](test/unit) and [integration](test/integration) tests is the tests themselves. One of the maintainers can help by pointing out relevant examples.
- Check your formatting and linting with [Flake8](https://flake8.pycqa.org/en/latest/#), [Black](https://github.com/psf/black), and the rest of the hooks we have in our [pre-commit](https://pre-commit.com/) [config](https://github.com/dbt-labs/dbt-core/blob/75201be9db1cb2c6c01fa7e71a314f5e5beb060a/.pre-commit-config.yaml).
ChenyuLInx marked this conversation as resolved.
Show resolved Hide resolved

In some cases, the right resolution to an open issue might be tangential to the `dbt-core` codebase. The right path forward might be a documentation update or a change that can be made in user-space. In other cases, the issue might describe functionality that the `dbt-core` maintainers are unwilling or unable to incorporate into the `dbt-core` codebase. When it is determined that an open issue describes functionality that will not translate to a code change in the `dbt-core` repository, the issue will be tagged with the `wontfix` label (see below) and closed.

Expand Down Expand Up @@ -106,6 +107,7 @@ A short list of tools used in `dbt-core` testing that will be helpful to your un
- [`pytest`](https://docs.pytest.org/en/latest/) to discover/run tests
- [`make`](https://users.cs.duke.edu/~ola/courses/programming/Makefiles/Makefiles.html) - but don't worry too much, nobody _really_ understands how make works and our Makefile is super simple
- [`flake8`](https://flake8.pycqa.org/en/latest/) for code linting
- [`black`](https://github.com/psf/black) for code formatting
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need black included in some kind of dev-requirements file or will pre-hooks handle that installation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Normally pre-commit does it for you but since we're using the language: system option we need to add it to dev reqs.

Fixed!

- [`mypy`](https://mypy.readthedocs.io/en/stable/) for static type checking
- [Github Actions](https://github.com/features/actions)

Expand Down Expand Up @@ -190,19 +192,18 @@ make test
# Runs postgres integration tests with py38 in "fail fast" mode.
make integration
```
> These make targets assume you have a recent version of [`tox`](https://tox.readthedocs.io/en/latest/) installed locally,
> These make targets assume you have a local install of a recent version of [`tox`](https://tox.readthedocs.io/en/latest/) for unit/integration testing and pre-commit for code quality checks,
> unless you use choose a Docker container to run tests. Run `make help` for more info.

Check out the other targets in the Makefile to see other commonly used test
suites.

#### `pre-commit`
[`pre-commit`](https.pre-commit.com) takes care of running all code-checks for formatting and linting. Run `make dev` to install `pre-commit` in your local environment. Once this is done you can use any of the linter-based make targets as well as a git pre-commit hook that will ensure proper formatting and linting.

#### `tox`

[`tox`](https://tox.readthedocs.io/en/latest/) takes care of managing virtualenvs and install dependencies in order to run
tests. You can also run tests in parallel, for example, you can run unit tests
for Python 3.7, Python 3.8, Python 3.9, `flake8` checks, and `mypy` checks in
parallel with `tox -p`. Also, you can run unit tests for specific python versions
with `tox -e py37`. The configuration for these tests in located in `tox.ini`.
[`tox`](https://tox.readthedocs.io/en/latest/) takes care of managing virtualenvs and install dependencies in order to run tests. You can also run tests in parallel, for example, you can run unit tests for Python 3.7, Python 3.8, and Python 3.9 checks in parallel with `tox -p`. Also, you can run unit tests for specific python versions with `tox -e py37`. The configuration for these tests in located in `tox.ini`.

#### `pytest`

Expand Down
70 changes: 46 additions & 24 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,43 @@ endif

.PHONY: dev
dev: ## Installs dbt-* packages in develop mode along with development dependencies.
pip install -r dev-requirements.txt -r editable-requirements.txt
@\
pip install -r dev-requirements.txt -r editable-requirements.txt && \
pre-commit install

.PHONY: mypy
mypy: .env ## Runs mypy for static type checking.
$(DOCKER_CMD) tox -e mypy
mypy: .env ## Runs mypy against staged changes for static type checking.
@\
$(DOCKER_CMD) pre-commit run --hook-stage manual mypy-check | grep -v "INFO"

.PHONY: flake8
flake8: .env ## Runs flake8 to enforce style guide.
$(DOCKER_CMD) tox -e flake8
flake8: .env ## Runs flake8 against staged changes to enforce style guide.
@\
$(DOCKER_CMD) pre-commit run --hook-stage manual flake8-check | grep -v "INFO"

.PHONY: black
black: .env ## Runs black against staged changes to enforce style guide.
@\
$(DOCKER_CMD) pre-commit run --hook-stage manual black-check -v | grep -v "INFO"

.PHONY: lint
lint: .env ## Runs all code checks in parallel.
$(DOCKER_CMD) tox -p -e flake8,mypy
lint: .env ## Runs flake8 and mypy code checks against staged changes.
@\
$(DOCKER_CMD) pre-commit run flake8-check --hook-stage manual | grep -v "INFO"; \
$(DOCKER_CMD) pre-commit run mypy-check --hook-stage manual | grep -v "INFO"

.PHONY: unit
unit: .env ## Runs unit tests with py38.
@\
$(DOCKER_CMD) tox -e py38

.PHONY: test
test: .env ## Runs unit tests with py38 and code checks in parallel.
$(DOCKER_CMD) tox -p -e py38,flake8,mypy
test: .env ## Runs unit tests with py38 and code checks against staged changes.
@\
$(DOCKER_CMD) tox -p -e py38; \
$(DOCKER_CMD) pre-commit run black-check --hook-stage manual | grep -v "INFO"; \
$(DOCKER_CMD) pre-commit run flake8-check --hook-stage manual | grep -v "INFO"; \
$(DOCKER_CMD) pre-commit run mypy-check --hook-stage manual | grep -v "INFO"

.PHONY: integration
integration: .env integration-postgres ## Alias for integration-postgres.
Expand All @@ -38,15 +54,18 @@ integration-fail-fast: .env integration-postgres-fail-fast ## Alias for integrat

.PHONY: integration-postgres
integration-postgres: .env ## Runs postgres integration tests with py38.
@\
$(DOCKER_CMD) tox -e py38-postgres -- -nauto

.PHONY: integration-postgres-fail-fast
integration-postgres-fail-fast: .env ## Runs postgres integration tests with py38 in "fail fast" mode.
@\
$(DOCKER_CMD) tox -e py38-postgres -- -x -nauto

.PHONY: setup-db
setup-db: ## Setup Postgres database with docker-compose for system testing.
docker-compose up -d database
@\
docker-compose up -d database && \
PGHOST=localhost PGUSER=root PGPASSWORD=password PGDATABASE=postgres bash test/setup_db.sh

# This rule creates a file named .env that is used by docker-compose for passing
Expand All @@ -62,26 +81,29 @@ endif

.PHONY: clean
clean: ## Resets development environment.
rm -f .coverage
rm -rf .eggs/
rm -f .env
rm -rf .tox/
rm -rf build/
rm -rf dbt.egg-info/
rm -f dbt_project.yml
rm -rf dist/
rm -f htmlcov/*.{css,html,js,json,png}
rm -rf logs/
rm -rf target/
find . -type f -name '*.pyc' -delete
find . -type d -name '__pycache__' -depth -delete
@echo 'cleaning repo...'
@rm -f .coverage
@rm -rf .eggs/
@rm -f .env
@rm -rf .tox/
@rm -rf build/
@rm -rf dbt.egg-info/
@rm -f dbt_project.yml
@rm -rf dist/
@rm -f htmlcov/*.{css,html,js,json,png}
@rm -rf logs/
@rm -rf target/
@find . -type f -name '*.pyc' -delete
@find . -type d -name '__pycache__' -depth -delete
@echo 'done.'


.PHONY: help
help: ## Show this help message.
@echo 'usage: make [target] [USE_DOCKER=true]'
@echo
@echo 'targets:'
@grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}'
@grep -E '^[8+a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}'
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an odd change I don't understand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because flake8 has an 8 in it and was escaping the help regex because of it. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

oh boy lololol

@echo
@echo 'options:'
@echo 'use USE_DOCKER=true to run target in a docker container'
2 changes: 2 additions & 0 deletions dev-requirements.txt
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
black==21.12b0
bumpversion
flake8
flaky
freezegun==0.3.12
ipdb
mypy==0.782
pip-tools
pre-commit
pytest
pytest-dotenv
pytest-logbook
Expand Down
21 changes: 1 addition & 20 deletions tox.ini
Original file line number Diff line number Diff line change
@@ -1,25 +1,6 @@
[tox]
skipsdist = True
envlist = py37,py38,py39,flake8,mypy

[testenv:flake8]
description = flake8 code checks
basepython = python3.8
skip_install = true
commands = flake8 --select=E,W,F --ignore=W503,W504,E741,E203 --max-line-length 99 \
core/dbt \
plugins/postgres/dbt
deps =
-rdev-requirements.txt

[testenv:mypy]
description = mypy static type checking
basepython = python3.8
skip_install = true
commands = mypy --show-error-codes core/dbt
deps =
-rdev-requirements.txt
-reditable-requirements.txt
envlist = py37,py38,py39

[testenv:{unit,py37,py38,py39,py}]
description = unit testing
Expand Down