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

Fix untyped functions in core/dbt/context/base.py #8525

Merged
merged 5 commits into from
Aug 31, 2023

Conversation

QMalcolm
Copy link
Contributor

resolves #8394

Problem

We have a lot of untyped functions in core/dbt/context/base.py this means that

  1. There is a lot of type checking that is not happening
  2. When a type issue occurs in the file a bunch of can't type check untyped functions warnings appear

Solution

ADD TYPES TO ALL THE THINGS 🎉

Checklist

  • I have read the contributing guide and understand what's expected of me
  • 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
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@cla-bot cla-bot bot added the cla:yes label Aug 30, 2023
@QMalcolm QMalcolm added the Skip Changelog Skips GHA to check for changelog file label Aug 30, 2023
@dbt-labs dbt-labs deleted a comment from github-actions bot Aug 30, 2023
@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Patch coverage: 98.93% and project coverage change: +1.77% 🎉

Comparison is base (d8e8a78) 84.58% compared to head (a613ef0) 86.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8525      +/-   ##
==========================================
+ Coverage   84.58%   86.36%   +1.77%     
==========================================
  Files         174      174              
  Lines       25579    25578       -1     
==========================================
+ Hits        21637    22090     +453     
+ Misses       3942     3488     -454     
Flag Coverage Δ
integration 83.16% <98.93%> (+2.05%) ⬆️
unit 65.03% <98.93%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
core/dbt/context/base.py 89.01% <97.82%> (-0.05%) ⬇️
core/dbt/context/configured.py 97.53% <100.00%> (ø)
core/dbt/context/docs.py 96.77% <100.00%> (ø)
core/dbt/context/manifest.py 100.00% <100.00%> (ø)
core/dbt/context/providers.py 88.66% <100.00%> (+0.86%) ⬆️
core/dbt/context/secret.py 79.16% <100.00%> (ø)
core/dbt/context/target.py 100.00% <100.00%> (ø)

... and 33 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@QMalcolm
Copy link
Contributor Author

QMalcolm commented Aug 30, 2023

I'm very confused on how the functions contextmember and contextproperty operate. They are function decorators, and in python function decorators are required to return a callable. However... in cases where the value passed into the decorator isn't a string, we return an object (ContextMember). Thus typing these two functions is neigh impossible because currently we have to state that one of the return types must be a bare ContextMember object, which then every invocation of the decorator complains about.

@QMalcolm QMalcolm force-pushed the qmalcolm--8394-fix-untyped-functions-in-context-basepy branch 3 times, most recently from 909b377 to 4243780 Compare August 31, 2023 20:02
@QMalcolm QMalcolm marked this pull request as ready for review August 31, 2023 20:03
@QMalcolm QMalcolm requested a review from a team as a code owner August 31, 2023 20:03
@QMalcolm QMalcolm requested a review from emmyoop August 31, 2023 20:03
@QMalcolm QMalcolm force-pushed the qmalcolm--8394-fix-untyped-functions-in-context-basepy branch from 4243780 to 48c7de0 Compare August 31, 2023 20:04
In addition to just adding parameter typing and return typing to
`BaseContext` functions. We also declared `_context_members_` and
`_context_attrs_` as properites of `BaseContext` this was necessary
because they're being accessed in the classes functions. However,
because they were being indirectly instantiated by the metaclass
`ContextMeta`, the properties weren't actually known to exist. By
adding declaring the properties on the `BaseContext`, we let mypy
know they exist.
… and add typing to them

Previously `contextmember` and `contextproperty` were 2-in-1 decorators.
This meant they could be invoked either as `@contextmember` or
`@contextmember('some_string')`. This was fine until we wanted to return
typing to the functions. In the instance where the bare decorator was used
(i.e. no `(...)` were present) an object was expected to be returned. However
in the instance where parameters were passed on the invocation, a callable
was expected to be returned. Putting a union of both in the return type
made the invocations complain about each others' return type. To get around this
we've dropped the bare invocation as acceptable. The parenthesis are now always
required, but passing a string in them is optional.
@QMalcolm QMalcolm force-pushed the qmalcolm--8394-fix-untyped-functions-in-context-basepy branch from 48c7de0 to a613ef0 Compare August 31, 2023 20:07
@QMalcolm QMalcolm merged commit e5e1a27 into main Aug 31, 2023
@QMalcolm QMalcolm deleted the qmalcolm--8394-fix-untyped-functions-in-context-basepy branch August 31, 2023 20:34
@McKnight-42
Copy link
Contributor

Out of curiosity is this a different way of approaching the change we reverted here #8306?

@QMalcolm
Copy link
Contributor Author

QMalcolm commented Sep 1, 2023

Out of curiosity is this a different way of approaching the change we reverted here #8306?

@McKnight-42 I'm not wholly sure, but I don't think so. I could absolutely be wrong about that though. My understanding of issue #8153 is that all numeric values were being forced into floats, and the solution (#8306) was to actually add an Integer object for creating integer numeric types. In this PR we're not adding or changing any functionality of the code itself (with the exception of a613ef0). The primary focus of this PR is defining what the expected return type of a function should be. That way when we run mypy, it can properly check all of our typing. These function return type definitions are actually thrown away at run time and don't actually affect the operation of the code.

@McKnight-42
Copy link
Contributor

ah makes sense thanks for clarification @QMalcolm

@emmyoop emmyoop added the user docs [docs.getdbt.com] Needs better documentation label Sep 6, 2023
@dbt-labs dbt-labs deleted a comment from FishtownBuildBot Sep 6, 2023
@emmyoop emmyoop removed the user docs [docs.getdbt.com] Needs better documentation label Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes Skip Changelog Skips GHA to check for changelog file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2978] Fix untyped functions in context/base.py (mypy warning)
4 participants