Add annotations to the code #1982
Conversation
Fokko
force-pushed
the
fd-add-annotations
branch
from
9bb3c69
to
2f17424
Compare
Fokko
force-pushed
the
fd-add-annotations
branch
from
8b2f6bb
to
ca45436
Compare
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Thanks @drewbanin I've signed the CLA about two hours ago. No worries about the spamming. Let me check the CI |
core/dbt/adapters/sql/impl.py
Outdated
| @@ -182,7 +184,7 @@ def drop_schema(self, database, schema): | |||
| self.execute_macro(DROP_SCHEMA_MACRO_NAME, | |||
| kwargs=kwargs) | |||
|
|
|||
| def list_relations_without_caching(self, information_schema, schema): | |||
| def list_relations_without_caching(self, information_schema, schema) -> List[BaseRelation]: | |||
There was a problem hiding this comment.
This line is too long and will end up failing pylint, it's too many characters! There are a other more lines like this too (119, 81, 77, 72)
| def list_relations_without_caching(self, information_schema, schema) -> List[BaseRelation]: | |
| def list_relations_without_caching( | |
| self, information_schema, schema | |
| ) -> List[BaseRelation]: |
There was a problem hiding this comment.
Thanks, I've set the line length to 80.
core/dbt/adapters/sql/impl.py
Outdated
| @@ -9,6 +9,7 @@ | |||
| from dbt.adapters.sql import SQLConnectionManager | |||
| from dbt.logger import GLOBAL_LOGGER as logger | |||
|
|
|||
| from core.dbt.adapters.factory import BaseRelation | |||
There was a problem hiding this comment.
| from core.dbt.adapters.factory import BaseRelation | |
| from dbt.adapters.factory import BaseRelation |
There was a problem hiding this comment.
What's the difference between core and non-core?
There was a problem hiding this comment.
dbt makes use of "namespace packages", as defined in PEP 420. The folder core contains one root namespace package in the dbt namespace, which you can install as dbt-core. The various plugin folders also contain root namespace packages in the dbt namespace - in particular they modify dbt.include and dbt.adapters by adding appropriate entries there.
So core is part of the folder structure and doesn't have any semantic meaning on the python side.
There was a problem hiding this comment.
Thanks for the explanation, appreciate it.
|
Looks good! I've kicked off the full test suite for this, I don't anticipate any failures. Once it passes we'll get this merged (likely for 0.15.1) |
core/dbt/adapters/sql/impl.py
Outdated
| return "text" | ||
|
|
||
| @classmethod | ||
| def convert_number_type(cls, agate_table, col_idx): | ||
| def convert_number_type(cls, agate_table: agate.Table, col_idx: int) -> str: |
There was a problem hiding this comment.
Ugh, I always forget this - our pylint is set to 79 chars, not 80, so this line (just barely) fails it.
| def convert_number_type(cls, agate_table: agate.Table, col_idx: int) -> str: | |
| def convert_number_type( | |
| cls, agate_table: agate.Table, col_idx: int | |
| ) -> str: |
There was a problem hiding this comment.
Whoops, just pushed a fix.
|
The successful azure tests for this PR are here: https://dev.azure.com/fishtown-analytics/dbt/_build/results?buildId=1142 |
|
My pleasure @beckjake Thank you all for developing DBT, I starting to fall in love with it :-) |
I was working on dbt-spark yesterday, and it was sometime a bit tricky to see what's going on. Having some additional annotations (especially in the adapter part) would really help for newcomers to get around.