Pre commit Hooks (black, flake8, mypy, etc) [CT-105] #4639

iknox-fa · 2022-01-27T22:39:40Z

resolves #3195 Jira CT-105

Description

Adds a selection of pre-commit hooks, most importantly the black formatter.
Moves non-environment dependent tests to pre-commit and out of tox (mypy and flake8)
To enable the pre-commit hooks simply make dev.

Checklist

I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR

.pre-commit-config.yaml

leahwicz · 2022-01-31T15:48:42Z

@iknox-fa have you tried running this yet to see what we need to fix up in our code base? I'm guessing we don't abide by this perfectly right now 😄

leahwicz · 2022-01-31T15:49:15Z

Also we should add something to the Contributing file so contributors know to expect this to fail during commits if they have issues

gshank · 2022-01-31T16:53:51Z

I'm guessing that this doesn't limit itself to the things that were just changes, so this should include a reformatting of the entire codebase, right?

In particular we have never event run flake8 on the test directories. I'm wondering if it might not be easier to skip the test/unit and test/integration directories for now, since they're going to change a lot soon.

iknox-fa · 2022-02-01T22:10:31Z

@iknox-fa have you tried running this yet to see what we need to fix up in our code base? I'm guessing we don't abide by this perfectly right now 😄

Its... a lot. 306 of 332 files were reformatted when I did a test run locally.

iknox-fa · 2022-02-01T22:22:00Z

Also we should add something to the Contributing file so contributors know to expect this to fail during commits if they have issues

Good call. Updated

iknox-fa · 2022-02-01T22:26:06Z

I'm guessing that this doesn't limit itself to the things that were just changes, so this should include a reformatting of the entire codebase, right?

Be default it just corrects files that you are currently trying to commit, but you can run it against everything of course. We will want to run it against everything just to minimize the confusion that might come from a PR touching so many files, but I'd like to do it in a separate PR.

In particular we have never event run flake8 on the test directories. I'm wondering if it might not be easier to skip the test/unit and test/integration directories for now, since they're going to change a lot soon.
I think we should go ahead and do everything, in theory black makes the testing code easier to read so it would be useful to do before you go refactor a ton of it.

kwigley

Love it! Can we please enforce these pre-commit hooks in CI and move the black config (line length) to a config file (pyproject.toml) so that IDEs, editors, and running black . directly on the cli can all use the same config?

iknox-fa · 2022-02-03T21:08:34Z

Love it! Can we please enforce these pre-commit hooks in CI and move the black config (line length) to a config file (pyproject.toml) so that IDEs, editors, and running black . directly on the cli can all use the same config?

pyproject.toml has further reaching implications than just linting stuff! That said I'm all for it (and it's the only way if you want to escape the hellscape of setuptools some day).

I went ahead and put this in another ticket, as it may cause issues with some of the other tools that build/interact with dbt.

kwigley

I think there are some references in CONTRIBUTING.md to tox and flake8/mypy, can you update this as well?

https://github.com/dbt-labs/dbt-core/blob/pre-commit-CT-105/CONTRIBUTING.md#tox

kwigley · 2022-02-08T17:17:05Z

tox.ini

@@ -2,25 +2,6 @@
 skipsdist = True
 envlist = py37,py38,py39,flake8,mypy


Suggested change

envlist = py37,py38,py39,flake8,mypy

envlist = py37,py38,py39

Good catch! Updated!

kwigley · 2022-02-08T17:24:40Z

.github/workflows/main.yml

-      - name: Run tox
-        run: tox
+      - name: Run pre-commit hooks
+        run: pre-commit


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.

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

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

failure singular! whomp whomp adding it now.

kwigley · 2022-02-08T17:31:55Z

Makefile

+	@\
+	$(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]" \


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

iknox-fa · 2022-02-08T21:56:21Z

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

CONTRIBUTING.md

iknox-fa · 2022-02-10T22:29:15Z

I think there are some references in CONTRIBUTING.md to tox and flake8/mypy, can you update this as well?

https://github.com/dbt-labs/dbt-core/blob/pre-commit-CT-105/CONTRIBUTING.md#tox

Whoops. Missed this comment initially. Fixed now!

kwigley

Looks good to me! I'd love if someone else from the team could take this for a spin as well 👍

nathaniel-may

Thanks for working to meet my linting needs on this one!

iknox-fa added 3 commits January 27, 2022 16:16

testing pre-commit hook

fb9cca1

test pre-commit 2

e9ac78e

adding pre-commit hook

3f48ea4

iknox-fa requested review from gshank, emmyoop, kwigley and leahwicz January 27, 2022 22:39

iknox-fa requested a review from a team as a code owner January 27, 2022 22:39

cla-bot bot added the cla:yes label Jan 27, 2022

return setup.py

45db18b

iknox-fa changed the title ~~Pre commit [CT-105]~~ Pre commit Hooks (black) [CT-105] Jan 27, 2022

return setup.py again

9fafac6

leahwicz reviewed Jan 31, 2022

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

PR feedback

75201be

PR feedback

a7d99a9

iknox-fa requested a review from leahwicz February 1, 2022 22:26

kwigley suggested changes Feb 3, 2022

View reviewed changes

adjust tox settings for flake8 to match black

4d44780

iknox-fa requested a review from kwigley February 4, 2022 15:23

iknox-fa added 4 commits February 4, 2022 09:52

adding flake8 to hooks

675d551

updates.. moar

35a6447

Makefile changes

d3865d7

Makefile Makeove! Squeee

3d65519

PR feedback

4f35cbf

kwigley suggested changes Feb 8, 2022

View reviewed changes

iknox-fa added 4 commits February 8, 2022 11:40

PR feedback

cb09add

PR feedback

64eede8

PR feedback

b7a6d01

PR feedback

522b4f7

ChenyuLInx reviewed Feb 8, 2022

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

iknox-fa added 5 commits February 8, 2022 16:54

PR feedback

7c13388

PR Feedback

a18c790

PR feedback

8477fdb

updated makefile, added pre-commit targets

33727ed

updated stages for checks

2161a53

iknox-fa requested review from kwigley, ChenyuLInx and nathaniel-may February 10, 2022 16:53

PR feedback

8af83a0

iknox-fa added 4 commits February 11, 2022 11:05

removed black from make lint (too slow)

2df7667

merge master

e5cf9fa

updating workflow to install mypy

e2f7f5a

updating workflow to install editable reqs

e64cc0a

kwigley approved these changes Feb 11, 2022

View reviewed changes

nathaniel-may approved these changes Feb 11, 2022

View reviewed changes

iknox-fa merged commit c112050 into main Feb 11, 2022

iknox-fa deleted the pre-commit-CT-105 branch February 11, 2022 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pre commit Hooks (black, flake8, mypy, etc) [CT-105] #4639

Pre commit Hooks (black, flake8, mypy, etc) [CT-105] #4639

iknox-fa commented Jan 27, 2022 •

edited

Loading

leahwicz commented Jan 31, 2022

leahwicz commented Jan 31, 2022

gshank commented Jan 31, 2022

iknox-fa commented Feb 1, 2022

iknox-fa commented Feb 1, 2022

iknox-fa commented Feb 1, 2022

kwigley left a comment

iknox-fa commented Feb 3, 2022

kwigley left a comment

kwigley Feb 8, 2022

iknox-fa Feb 8, 2022

kwigley Feb 8, 2022

iknox-fa Feb 8, 2022

kwigley Feb 8, 2022

iknox-fa Feb 8, 2022

kwigley Feb 8, 2022

iknox-fa commented Feb 8, 2022

iknox-fa commented Feb 10, 2022

kwigley left a comment •

edited

Loading

nathaniel-may left a comment

		@@ -2,25 +2,6 @@
		skipsdist = True
		envlist = py37,py38,py39,flake8,mypy

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

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

Pre commit Hooks (black, flake8, mypy, etc) [CT-105] #4639

Pre commit Hooks (black, flake8, mypy, etc) [CT-105] #4639

Conversation

iknox-fa commented Jan 27, 2022 • edited Loading

Description

Checklist

leahwicz commented Jan 31, 2022

leahwicz commented Jan 31, 2022

gshank commented Jan 31, 2022

iknox-fa commented Feb 1, 2022

iknox-fa commented Feb 1, 2022

iknox-fa commented Feb 1, 2022

kwigley left a comment

Choose a reason for hiding this comment

iknox-fa commented Feb 3, 2022

kwigley left a comment

Choose a reason for hiding this comment

kwigley Feb 8, 2022

Choose a reason for hiding this comment

iknox-fa Feb 8, 2022

Choose a reason for hiding this comment

kwigley Feb 8, 2022

Choose a reason for hiding this comment

iknox-fa Feb 8, 2022

Choose a reason for hiding this comment

kwigley Feb 8, 2022

Choose a reason for hiding this comment

iknox-fa Feb 8, 2022

Choose a reason for hiding this comment

kwigley Feb 8, 2022

Choose a reason for hiding this comment

iknox-fa commented Feb 8, 2022

iknox-fa commented Feb 10, 2022

kwigley left a comment • edited Loading

Choose a reason for hiding this comment

nathaniel-may left a comment

Choose a reason for hiding this comment

iknox-fa commented Jan 27, 2022 •

edited

Loading

kwigley left a comment •

edited

Loading