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

Adding Python 3.10 to test matrix #177

Merged
merged 7 commits into from
May 18, 2022
Merged

Adding Python 3.10 to test matrix #177

merged 7 commits into from
May 18, 2022

Conversation

leahwicz
Copy link
Contributor

@leahwicz leahwicz commented May 1, 2022

Description

This change started out as Python 3.10 support but then grew some. With this change, we now will have:

  • Python 3.10 tests running for CI
  • mypy upgraded to match Core
  • mypy enabled for the repo
  • third party stubs (needed for mypy)

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

@cla-bot cla-bot bot added the cla:yes label May 1, 2022
@gshank
Copy link
Contributor

gshank commented May 5, 2022

You can't add an init.py file underneath the 'dbt' directory. It breaks the plugin architecture.

@leahwicz leahwicz marked this pull request as ready for review May 5, 2022 21:55
@@ -0,0 +1,3 @@
from pkgutil import extend_path

__path__ = extend_path(__path__, __name__)
Copy link
Contributor

@VersusFacit VersusFacit May 6, 2022

Choose a reason for hiding this comment

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

nit: I've read changing path this way is uncommon. What's the reason here? Super curious about the use case. edit: Does it have to do with Gerda's comment by chance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had a convo in Slack here about it but the TLDR is that we reuse dbt namespace twice which confuses mypy. This helps just combine them for mypy to reference. In the long run, we should refactor the codebase to not have this issue

Copy link
Contributor

@VersusFacit VersusFacit left a comment

Choose a reason for hiding this comment

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

In a previous effort, I played around with notions of mypy's added stub feature. I opted away from this after having several nontrivial process questions. Instead, I type: ignore-ed several things.

Here are some questions that come to mind:

  • where all the agate stubs you've included came from!
  • How do we purpose to keep this up to date with the changing library specification?
  • What's the update procedure?

@leahwicz
Copy link
Contributor Author

leahwicz commented May 6, 2022

Good questions Mila!

where all the agate stubs you've included came from!

I copied them from dbt-core where they already exist.

How do we purpose to keep this up to date with the changing library specification? / What's the update procedure?

In the majority of the cases, we should use the associated types package for each of these dependencies. I removed the response stubs I originally created myself and switched the types-requests package instead. This is the way we can keep up to date with dependencies over time. For dependencies that don't provide the associated types package, we don't have a good plan in place at the moment which is a gap. Agate is a dependency in general we want to remove in general. I'll take a look at the stubs we have in all the repos and start thinking if there is a better approach there. We can start a thread with the team too since I'm curious if others have seen this done in a certain way in the past

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, not seeing any red-flags sticking out in the types

@leahwicz leahwicz merged commit 8240522 into main May 18, 2022
@leahwicz leahwicz deleted the leahwicz/py310 branch May 18, 2022 21:45
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.

4 participants