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

Add pre commit hooks and format changes in repo. #99

Closed
wants to merge 6 commits into from

Conversation

VersusFacit
Copy link
Contributor

@VersusFacit VersusFacit commented Mar 2, 2022

resolves #95

Description

This repository outfits the Snowflake repository with new CI/CD utilities, dev tools (e.g. Makefile), and formats source herein according to these tools.

Things went mostly without problem. However, there exists a lingering set of questions before merging. These are more "what do we want" questions as opposed to "help me with this bug."

Spec-related questions, only the first of which is a non-trivial manner. The rest don't have much in the way of tradeoffs:

  1. MyPy is failing because of a duplicate module name (damn you Python internal "package", cf. module, manager). Do we want to resolve this by renaming the directory paths to resolve this conflicting module error or turn off so-called 'misc' errors in mypy. Some trade offs follow:
    • Turning off MyPy is probably a nonstarter.
    • Turning off misc errors is too "greedy" in its nullification of errors
    • Renaming the directories and files to resolve module conflicts would solve this once and for all. Although we may miss the mirrored structure against dbt-core, I am leaning this way.

image

  1. Do we want to manually alter strings and commits which Black cannot format? In the event, they exceed 99 characters, should we manually format them?
  2. The Makefile has several docker references. I don't think Snowflake is something worth interacting with through Docker (Unless like, sandboxing dependencies further is a big gain). Hence, we should probably not implement this for this or other Cloud warehouse repoes. Yay or nay?
  3. What checks do we want on tests right now? I'm assuming none, per guidance in Ian's core PR about us doing that after the overhaul. Sanity check -- does that sound right? I can edit the yaml to reflect this or another strategy we may have.

For viewing convenience, the first commit in this PR handles the commit hook logic. Everything else are batches of file changes.

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
  • I have updated the CHANGELOG.md and added information about my change to the "dbt-snowflake next" section.

@VersusFacit
Copy link
Contributor Author

VersusFacit commented Mar 2, 2022

My own thoughts:

  1. we should change directory structure because it is a solution using the idioms of python. It a package problem centered around the init.py's (i.e. "package" structure of the language).
  2. I think we should manually address these.
  3. I'm guessing this isn't possible because it's a snowflake adapter (i.e. not an on prem db) and this violates my understanding of Docker. Please correct if I'm wrong. The Makefile is a nice though.
  4. I think we should probably do nothing to reduce unnecessary work.

@McKnight-42
Copy link
Contributor

McKnight-42 commented Mar 2, 2022

Hey figured id share some of my findings seems we are having some of the same snags.

  1. I've tried following a few guidelines on this i've tried so far I've tried adding some excludes to a mypy.ini and adding pass_filenames: false to pre-commit.yaml Suppress 'duplicate module named xx' errors? python/mypy#4008 which haven't yet yielded helpful results.
  2. currently to get this to pass on the flake8 side i'm adding E501 to the ignore in .flake8 to ignore the long line changes as most of these are comments that black won't break up.
  3. I'm in agreement that the Makefile is not likely needed but docker necessities are something i'm a little fuzzy on.
  4. Me and @kwigley actually tried to debug this one pretty hard was only make the docs_generate tests fail and seems to be tied to the end-of-file-fixer changes black makes, was unable to find a good solution so did wind up applying the global exclude as well.

@VersusFacit VersusFacit marked this pull request as draft March 2, 2022 17:38
@VersusFacit
Copy link
Contributor Author

VersusFacit commented Mar 2, 2022

Thanks for the background, Matt. Good to see some additional context on these matters.

That said, we'll have to see what Ian jumps in with in order to know what solutions would best meet the intended spec. At the moment, things could go "either" way for lots of these.

@leahwicz
Copy link
Contributor

leahwicz commented Mar 3, 2022

@VersusFacit @McKnight-42 good questions here! Let me give my 2 cents:

  1. Let's ask @iknox-fa about any mypy issues he ran into and what he did there. Renaming sounds right to me though. I would do that in separate PR and not keep the changes growing in this one.

  2. I don't think we did this in the Core repo so I wouldn't do it here.

  3. Good catch! The Docker references are probably just copy/pasted code leftover from the repo split and can be removed. Let's clean that up in a separate PR then

  4. We should replace the flake8 check with the code-quality check that you can see here.

@VersusFacit VersusFacit closed this Mar 8, 2022
@VersusFacit VersusFacit deleted the CT-177/add_precommit_hooks branch March 8, 2022 00:15
@iknox-fa
Copy link
Contributor

iknox-fa commented Mar 8, 2022

Sorry it's taken me a bit to get back to you @VersusFacit and co. Here's my $.02

MyPy is failing because of a duplicate module name...
Renaming the directories and files to resolve module conflicts would solve this once and for all. Although we may miss the mirrored structure against dbt-core, I am leaning this way.

Agreed, there is a third option which is to dive into the mysteries of MyPy to be able to dictate how mypy reads our codebases... in theory we can "make it understand" that they're different things. It's work we're going to need to do eventually on core, but probably doesn't make sense in adapters just yet.

Do we want to manually alter strings and commits which Black cannot format? In the event, they exceed 99 characters, should we manually format them?

Yes please :)

Makefile has several docker references

Let's punt on that for now. All the core repos makefiles have those refs since for a long time we had a docker-based dev workflow. I'd want to make sure it's not being used elsewhere (in cloud maybe? /shrug) before we nuke it. When we do nuke it we should do so for all repos that have Makefiles

What checks do we want on tests right now?

Maybe? My recommendation was based on the current work for core that's re-vamping the way tests are run. If that work is going to be done here as well then yeah, skip it. Otherwise it might be worth putting some effort into it. All in all having tests pass mypy isn't a very high priority IMO.

This was referenced Mar 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-239] Add black formatter and precommit hooks
4 participants