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 19 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
W504
E203
Copy link
Contributor

Choose a reason for hiding this comment

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

503 and 203 are new compared to our last config. What's the reason for adding new ignores?

I also wouldn't hate if these were commented since it looks like we have a convenient place to put comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

203 and 503 are due to incompatibilities between flake8 and black. I'll throw in a few comments (although I can't speak for the other ignores)

Copy link
Contributor

Choose a reason for hiding this comment

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

keeping the others for consistency in this PR is good for me.

E741
max-line-length = 99
exclude = test
19 changes: 5 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,12 @@ jobs:
- name: Install python dependencies
run: |
pip install --user --upgrade pip
pip install tox
pip install pre-commit
pip --version
tox --version
pre-commit --version

- name: Run tox
run: tox
- name: Run pre-commit hooks
run: pre-commit
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be something like:

Suggested change
run: pre-commit
run: pre-commit --all-files --show-diff-on-failure

There are no staged files when this workflow runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh good catch (show-diff-on-failures doesn't seem to be a thing however). Fixed!

Copy link
Contributor

Choose a reason for hiding this comment

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

nice! and more on --show-diff-on-failure here. totally optional but I've found it helpful in CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

failure singular! whomp whomp adding it now.


unit:
name: unit test / python ${{ matrix.python-version }}
Expand Down
13 changes: 6 additions & 7 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# **what?**
# Take the given commit, run unit tests specifically on that sha, build and
# Take the given commit, run unit tests specifically on that sha, build and
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this whole file is just whitespace changes

# package it, and then release to GitHub and PyPi with that specific build

# **why?**
Expand Down Expand Up @@ -142,9 +142,8 @@ jobs:
run: |
dbt --version


github-release:
name: GitHub Release
name: GitHub Release

needs: test-build

Expand All @@ -155,7 +154,7 @@ jobs:
with:
name: dist
path: '.'

# Need to set an output variable because env variables can't be taken as input
# This is needed for the next step with releasing to GitHub
- name: Find release type
Expand All @@ -179,7 +178,7 @@ jobs:
dbt_core-${{github.event.inputs.version_number}}-py3-none-any.whl
dbt-postgres-${{github.event.inputs.version_number}}.tar.gz
dbt-core-${{github.event.inputs.version_number}}.tar.gz

pypi-release:
name: Pypi release

Expand All @@ -188,12 +187,12 @@ jobs:
needs: github-release

environment: PypiProd
steps:
steps:
- uses: actions/download-artifact@v2
with:
name: dist
path: 'dist'

- name: Publish distribution to PyPI
uses: pypa/[email protected]
with:
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
42 changes: 42 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# 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

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
language_version: python3
args: ['--line-length=99']
- repo: https://gitlab.com/pycqa/flake8
rev: 4.0.1
hooks:
- id: flake8
- repo: https://github.com/pre-commit/mirrors-mypy
rev: v0.782
hooks:
- id: mypy
args: [--show-error-codes]
files: ^core/dbt/
entry: mypy core/dbt/
pass_filenames: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Well this is annoyingly specific. Since you had trouble getting here it might be worth a comment to save a future poor soul.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

# N.B.: 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.
language: system
2 changes: 2 additions & 0 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
64 changes: 43 additions & 21 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,44 @@ 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
@\
$(DOCKER_CMD) pre-commit run mypy | grep -v "[INFO]"

.PHONY: flake8
flake8: .env ## Runs flake8 to enforce style guide.
$(DOCKER_CMD) tox -e flake8
@\
$(DOCKER_CMD) pre-commit run flake8 | grep -v "[INFO]"

.PHONY: black
black: .env ## Runs black to enforce style guide.
@\
$(DOCKER_CMD) pre-commit run black | grep -v "[INFO]"

.PHONY: lint
lint: .env ## Runs all code checks in parallel.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this still run them all in parallel? that was a tox thing but I still want them to be run in parallel.

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 question. I thought it was a makefile thing, but I may be mistaken. I'll research and get back to ya.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this ends up being a bit of a big question. Here's the TL;DR--

  • Make, pre-commit, and the tools themselves all support some sort of parallelism... to get the best performance here we want to use exactly one method.
  • Pre-commit's parallelism is based on splitting the work to be done based on batching requests to each hook based on the number of files. This makes the most sense to optimize for since file batch control is one of the best features pre-commit has.
  • I've confirmed that it works.. PStree output showing black doing it's thing below.
  • All that said, if it's not fast enough, it's not fast enough so I'm removing it from the PR.

$(DOCKER_CMD) tox -p -e flake8,mypy
@\
$(DOCKER_CMD) pre-commit run flake8 | grep -v "[INFO]"; \
$(DOCKER_CMD) pre-commit run mypy | grep -v "[INFO]"; \
$(DOCKER_CMD) pre-commit run black | grep -v "[INFO]" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean make lint modifies our code now via black?, Since I run this every 2 seconds while developing I would personally benefit from something like make lint-check that doesn't modify source or pulling black out of the lint command.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will only run on git staged files. Is that what is intended here?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh I def want it to run on everything. I never stage before running make lint

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 sounds like we have some different use cases for how we use make lint.

A: Yes black is now included in the linter step and it will make changes. I get how that's not ideal so I can change it to just output the errors/warnings without making changes.

B: I had imagined using it just for changes.. but it that's not the way we want it to go I'm happy to change it to run on everything.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe keep make lint as close to the same as it was, but have a new command make format or make pre-commit or whatever that does all the things on staged files? 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, I never use make lint and no-one else has said anything, so I think you get to pick how you want it to work @nathaniel-may

Copy link
Contributor

Choose a reason for hiding this comment

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

make lint: runs mypy and flake8 in parallel on all source files but makes no changes to them.

Copy link
Contributor Author

@iknox-fa iknox-fa Feb 10, 2022

Choose a reason for hiding this comment

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

There's a fair bit going on in terms of ways we use this so here's a matrix to describe the current state.

command hooks run files checked writes code changes
make dev* - - -
make black
make flake8
make mypy
linter/formatter
(as specified)
staged changes no
make lint flake8, mypy staged changes no
make test black, flake8, mypy staged changes no
git commit check-yaml, check-json,
fix-end-of-files,
trim-trailing-whitespace,
check-for-case-conflicts,
black, flake8, mypy
staged changes yes**
pre-commit run --all-files check-yaml, check-json,
fix-end-of-files,
trim-trailing-whitespace,
check-for-case-conflicts,
black, flake8, mypy
all files yes**
  • installs pre-commit hooks, required before any of this will work.
    ** except mypy and flake8, which don't support that feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finding a way to run this on unstaged changes without running it on everything in the entire repo is proving a bit challenging so I'm going to put that in as a tech debt ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed black from make lint and updated the matrix above.


.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
@\
$(DOCKER_CMD) tox -p -e py38; \
$(DOCKER_CMD) pre-commit run flake8 | grep -v "[INFO]"; \
$(DOCKER_CMD) pre-commit run mypy | grep -v "[INFO]"; \
$(DOCKER_CMD) pre-commit run black | grep -v "[INFO]"

.PHONY: integration
integration: .env integration-postgres ## Alias for integration-postgres.
Expand All @@ -38,15 +55,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,27 +82,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'

1 change: 1 addition & 0 deletions dev-requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ freezegun==0.3.12
ipdb
mypy==0.782
pip-tools
pre-commit
pytest
pytest-dotenv
pytest-logbook
Expand Down
19 changes: 0 additions & 19 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,6 @@
skipsdist = True
envlist = py37,py38,py39,flake8,mypy
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
envlist = py37,py38,py39,flake8,mypy
envlist = py37,py38,py39

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! Updated!


[testenv:flake8]
description = flake8 code checks
basepython = python3.8
skip_install = true
commands = flake8 --select=E,W,F --ignore=W504,E741 --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

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