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

Make pre commit code changes #345

Merged
merged 4 commits into from
May 11, 2022

Conversation

VersusFacit
Copy link
Contributor

resolves #292

Description

This PR is a combination of:

  1. auto formatting with pre-commit
  2. manual static annotation and library changes -- labelled throughout PR inline

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-spark next" section.

@VersusFacit VersusFacit self-assigned this May 5, 2022
@cla-bot cla-bot bot added the cla:yes label May 5, 2022
@VersusFacit VersusFacit mentioned this pull request May 5, 2022
4 tasks
@@ -11,7 +11,7 @@ flaky
freezegun==0.3.9
ipdb
mock>=1.3.0
mypy==0.782
mypy==0.950
Copy link
Contributor Author

@VersusFacit VersusFacit May 5, 2022

Choose a reason for hiding this comment

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

MyPy would not pass the way the code was written. Why? MyPy has a delineation between Type Variables and TypeAliass. To force dynamic classes such as Relation to be read as TypeAliass , we need a newer version of MyPy for use of type_extensions.

If anyone goes digging, they'll find that eventually typing gets TypeAlias as a native member in later versions of Python, but this doesn't include all dbts supported versions of Python. This solution was selected as the more surgical approach. I had begun to rewrite Spark-specific code but this felt really, really unsatisfactory and would have taken up needed development time. Hence, this.

Thankfully, changing the MyPy version doesn't result in any negative effects (I ran this over --all-files to be sure).

@VersusFacit VersusFacit force-pushed the make_pre-commit_code_changes branch from 82935c8 to 27681c5 Compare May 6, 2022 07:45
@VersusFacit VersusFacit requested review from jtcohen6 and ChenyuLInx and removed request for jtcohen6 May 6, 2022 08:09
@leahwicz leahwicz requested a review from McKnight-42 May 9, 2022 14:51
Copy link
Contributor

@McKnight-42 McKnight-42 left a comment

Choose a reason for hiding this comment

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

Looks good to me. thanks for picking this up and fantastic work!

if 'temporarily_unavailable' in message:
return exc.message
return None
def _is_retryable_error(exc: Exception) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice refactor!

For places like this where it's a refactor and not just formatting, can you call those out in the comments to make reviewing easier? Then it's easy to see what needs a closer look vs some newlines or spacing

Copy link
Contributor

@leahwicz leahwicz left a comment

Choose a reason for hiding this comment

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

LGTM!

@VersusFacit VersusFacit merged commit 8a659a7 into add_precommit_hook_logic May 11, 2022
@VersusFacit VersusFacit deleted the make_pre-commit_code_changes branch May 11, 2022 18:54
VersusFacit added a commit that referenced this pull request May 11, 2022
* Make pre commit code changes (#345)
* Refactor exception function using accurate type specs.
* Upgrade the mypy version in order to use TypeAlias-es.
* Upgrade the mypy version in the worfklow. Fix bug introduced by using str's instead of Optional[str]'s
* Address code review comments: Remove integration test command since there are multiple ways we can run tests in spark.
@VersusFacit VersusFacit mentioned this pull request May 11, 2022
4 tasks
VersusFacit added a commit that referenced this pull request May 13, 2022
* Add commit hook tooling. (#346)
* Make pre commit code changes (#345)
* Refactor exception function using accurate type specs.
* Upgrade the mypy version in order to use TypeAlias-es.
* Upgrade the mypy version in the worfklow. Fix bug introduced by using str's instead of Optional[str]'s
* Address code review comments: Remove integration test command since there are multiple ways we can run tests in spark.
* Add changelog entry
* Altering names of dev_requirements references.
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.

3 participants