adding pre-commit install to make dev

[justbldwn]

resolves #6269

Description

hey @dbeatty10 , I am opening this as a draft PR to review in reference to #6269. i can convert this to no longer be a draft if you are okay with this as a possible solution.

I noticed #6269 was open for adding pre-commit install to the CONTRIBUTING.md file. interestingly enough, the docs mention that make dev should install pre-commit for you here in the second sentence, but it's not currently doing that.

so far, this PR just adds pre-commit install as part of the make dev command in the Makefile, but I can also help add some docs to the CONTRIBUTING.md file to install pre-commit manually as an alternative to using the make dev command if you think it is necessary. i hope it's okay that this change is a bit out of scope for the requirements of the ticket, but this change should help contributors get setup more quickly to dbt-core as long as they are using the make commands.

let me know if there is anything else i can help contribute to this!

Checklist

• I have read the contributing guide and understand what's expected of me
• 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
• I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
• I have run changie new to create a changelog entry

[dbeatty10]

Thanks for picking this up @justbldwn! This is looking great ✨ -- let's run with it.

An argument could be made for creating a stand-alone rule within the Makefile for the pre-commit install, but I appreciate how your addition makes the minimal amount of changes to achieve the goals stated in #6269. Let's keep your approach as-is.

Two small suggestions:

1. split the commands over two lines using && \ (like here) to match the existing conventions
2. copy-paste the final commands here (similar to here)

Feel free to click the "Ready for review" button as soon as you're up for it 👍

[gshank]

For those who don't want pre-commit run automatically, could you add an additional make target (maybe "dev_req" that doesn't install pre-commit? Thanks.

[justbldwn]

For those who don't want pre-commit run automatically, could you add an additional make target (maybe "dev_req" that doesn't install pre-commit? Thanks.

thanks for the feedback! i just added a dev_req script to the Makefile and added some details to the CONTRIBUTING.md for instructions of with and without pre-commit.

[dbeatty10]

For those who don't want pre-commit run automatically

@gerda in which cases would we not want to install and run pre-commit? It seems like such a case would be the (rare) exception rather than the rule, no?

[dbeatty10]

Sorry for any churn here @justbldwn and @gshank

Hopefully we can come to a happy medium that:

1. Gives explicit instructions how to install all the expected tooling
2. Doesn't end up with longer and longer contributing file

My spicy take is that the contributing file should be closer to 100 lines long than 250 lines long so that it's more readable 🌶️ That's obviously outside of the focus of this PR, but still on my mind!

[dbeatty10]

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

[dbeatty10]

@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?

[gshank]

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

[dbeatty10]

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.

[justbldwn]

@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.

[dbeatty10]

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

[justbldwn]

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)

[dbeatty10]

There's two little pieces I didn't notice earlier related to .PHONY targets.

I'm adding them as suggestions in this review, and then I'll go ahead and commit them afterwards.

[dbeatty10]

This looks ready to rock 🎸

Thanks for this contribution @justbldwn !

[dbeatty10]

Could you re-review @gerda?

One thing to take a look at: the changlog entry currently says kind: Fixes

Should it be kind: Docs or something else instead?