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

adding pre-commit install to make dev #6417

Merged
merged 7 commits into from
Jan 3, 2023
+19 −4
Merged

adding pre-commit install to make dev #6417

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f7d2682
:sparkles: adding pre-commit install to make dev
justbldwn Dec 10, 2022
d3c7979
:art: updating format of Makefile and CONTRIBUTING.md
justbldwn Dec 12, 2022
7ee9fb3
:memo: adding changelog via changie new
justbldwn Dec 12, 2022
a553516
:sparkles: adding dev_req to Makefile + docs
justbldwn Dec 12, 2022
b4bf3b4
:art: remove dev_req from docs, dry makefile
justbldwn Dec 21, 2022
4d6945f
Align names of `.PHONY` targets with their associated rules
dbeatty10 Dec 23, 2022
39c7f81
Merge branch 'main' into precommit
dbeatty10 Dec 23, 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
7 changes: 7 additions & 0 deletions .changes/unreleased/Fixes-20221212-115912.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: Fixes
body: add pre-commit install to make dev script in Makefile
time: 2022-12-12T11:59:12.175136-05:00
custom:
Author: justbldwn
Issue: "6269"
PR: "6417"
18 changes: 16 additions & 2 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,25 @@ brew install postgresql

### Installation

First make sure that you set up your `virtualenv` as described in [Setting up an environment](#setting-up-an-environment). Also ensure you have the latest version of pip installed with `pip install --upgrade pip`. Next, install `dbt-core` (and its dependencies) with:
First make sure that you set up your `virtualenv` as described in [Setting up an environment](#setting-up-an-environment). Also ensure you have the latest version of pip installed with `pip install --upgrade pip`. Next, install `dbt-core` (and its dependencies) either with or without `pre-commit`:


#### Installation with `pre-commit`:
```sh
make dev
# or
```
or, alternatively:
```sh
pip install -r dev-requirements.txt -r editable-requirements.txt
pre-commit install
```

#### Installation without `pre-commit`:
```sh
make dev_req
```
or, alternatively:
```sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the addition of the dev_req rule, adding this section makes sense on first blush.

But... the contributing file already seems unbearably long to me. I wish it was like 75% shorter rather than even just a few lines longer.

Anyone that knows good reasons why they don't want the pre-commit install step probably has keen enough eyes to know that they can omit it and just run:

pip install -r dev-requirements.txt -r editable-requirements.txt

This is a case in which "less is more" and we can just omit explicitly documenting how to do it.

Suggested change
#### Installation without `pre-commit`:
```sh
make dev_req
```
or, alternatively:
```sh

Copy link
Contributor

Choose a reason for hiding this comment

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

@gshank I would prefer to exclude additional instructions to describe how to omit installation of pre-commit hooks for three main reasons:

  1. Installing pre-commit hooks is the happy path!
  2. How to omit installation of pre-commit seems obvious without explanation needed
  3. More text makes it harder to read the document as a whole

How would you feel about going back to the more simple instructions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see no need to describe how to use dev_req. Just have a Makefile target.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good @gshank!

@justbldwn If you commit my two suggestions (including the one to remove the "Installation without pre-commit" section), I think we'll be good to go!

You'll probably want to make a final pass through your edits to make sure everything reads the way you want it to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dbeatty10 @gshank thank you both for the helpful feedback here! i just pushed up a new commit to remove a chunk from the CONTRIBUTING.md about using make dev_req, and instead just left instructions for using make dev. however, the dev_req target in the Makefile still exists for anyone that may want to use it.

pip install -r dev-requirements.txt -r editable-requirements.txt
```

Expand Down
8 changes: 7 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,13 @@ CI_FLAGS =\
DBT_LOG_FORMAT=json

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

Choose a reason for hiding this comment

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

I'm not a Make guru, but it seems like this would be more DRY if re-factored like this:

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

Copy link
Contributor Author

@justbldwn justbldwn Dec 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

i like this a lot - thanks for the tip! hadn't considered this, but it makes total sense this way. it should now be written this way in the latest commit (b4bf3b4)


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

Expand Down