Convert to `attrs`, remove `runtype` #720

nolar · 2023-09-29T15:59:24Z

Short-term goal: resolve an issue where runtype cannot check isinstance() check, where the type is a string "Database" coming from Compilar.database: "Database".

Technically, it is a typing.ForwardRef[] and must be resolved to actual classes on usage. attrs and pydantic do that.

There is no way to convert it to a regular class, because there is still a circular class dependency Database <> Compiler.

Long-term goal: Since we are moving in the direction of strict type checking, let's make the first step: replace runtype with lots of implicit logic and not compatible with MyPy, with attrs, which require explicitness and work fine with MyPy & IDEs.

attrs is much more beneficial:

attrs is supported by type checkers, such as MyPy & IDEs
attrs is widely used and industry-proven
attrs is explicit in its declarations, there is no magic
attrs has slots

But mainly for the first item — type checking by type checkers.

And since we switch to attrs for already dataclass-like types, also expand it to all other (unannotated) classes to ensure proper attribute management in multiple inheritance.

In particular, in the last commit, there are a few cases where the code was ambiguous and had to be resolved by minor remakes.

It is better to review commit-by-commit.

github-actions · 2023-09-29T16:00:02Z

data_diff/abcs/database_types.py

 class PrecisionType(ColType):
    precision: int
    rounds: Union[bool, Unknown] = Unknown


+


[blackfmt] _{reported by reviewdog 🐶}

Suggested change

github-actions · 2023-09-29T16:00:02Z

data_diff/databases/_connect.py

 class Connect:
    """Provides methods for connecting to a supported database using a URL or connection dict."""
+    database_by_scheme: Dict[str, Database]


[blackfmt] _{reported by reviewdog 🐶}

Suggested change

database_by_scheme: Dict[str, Database]

database_by_scheme: Dict[str, Database]

github-actions · 2023-09-29T16:00:03Z

data_diff/databases/base.py

-    def __init__(self, compiler: Compiler, gen: Generator):
-        self.gen = gen
-        self.compiler = compiler
+    compiler: Compiler


[blackfmt] _{reported by reviewdog 🐶}

Suggested change

compiler: Compiler

compiler: Compiler

github-actions · 2023-09-29T16:00:03Z

data_diff/databases/base.py

@@ -522,7 +525,7 @@ def render_select(self, parent_c: Compiler, elem: Select) -> str:

    def render_join(self, parent_c: Compiler, elem: Join) -> str:
        tables = [
-            t if isinstance(t, TableAlias) else TableAlias(t, parent_c.new_unique_name()) for t in elem.source_tables
+            t if isinstance(t, TableAlias) else TableAlias(source_table_=t, name=parent_c.new_unique_name()) for t in elem.source_tables


[blackfmt] _{reported by reviewdog 🐶}

Suggested change

t if isinstance(t, TableAlias) else TableAlias(source_table_=t, name=parent_c.new_unique_name()) for t in elem.source_tables

t if isinstance(t, TableAlias) else TableAlias(source_table_=t, name=parent_c.new_unique_name())

for t in elem.source_tables

github-actions · 2023-09-29T16:00:03Z

data_diff/databases/base.py

        select = (
-            f"SELECT {columns_str} FROM {self.compile(c.replace(in_select=True), elem.table)} GROUP BY {keys_str}{having_str}"
+            f"SELECT {columns_str} FROM {self.compile(attrs.evolve(c, in_select=True), elem.table)} GROUP BY {keys_str}{having_str}"
        )


[blackfmt] _{reported by reviewdog 🐶}

Suggested change

select = (

f"SELECT {columns_str} FROM {self.compile(c.replace(in_select=True), elem.table)} GROUP BY {keys_str}{having_str}"

f"SELECT {columns_str} FROM {self.compile(attrs.evolve(c, in_select=True), elem.table)} GROUP BY {keys_str}{having_str}"

)

select = f"SELECT {columns_str} FROM {self.compile(attrs.evolve(c, in_select=True), elem.table)} GROUP BY {keys_str}{having_str}"

github-actions · 2023-09-29T16:00:04Z

data_diff/queries/ast_classes.py

-@dataclass
-class Param(ExprNode, ITable):
+@attrs.define(eq=False, kw_only=True)
+class Param(ExprNode, ITable):  # TODO: Unused?
    """A value placeholder, to be specified at compilation time using the `cv_params` context variable."""
    name: str


[blackfmt] _{reported by reviewdog 🐶}

Suggested change

name: str

name: str

github-actions · 2023-09-29T16:00:05Z

data_diff/thread_utils.py

@@ -31,19 +33,22 @@ class PriorityThreadPoolExecutor(ThreadPoolExecutor):

    XXX WARNING: Might break in future versions of Python
    """
-
    def __init__(self, *args):


[blackfmt] _{reported by reviewdog 🐶}

Suggested change

def __init__(self, *args):

def __init__(self, *args):

github-actions · 2023-09-29T16:00:05Z

data_diff/thread_utils.py

 class ThreadedYielder(Iterable):
    """Yields results from multiple threads into a single iterator, ordered by priority.

    To add a source iterator, call ``submit()`` with a function that returns an iterator.
    Priority for the iterator can be provided via the keyword argument 'priority'. (higher runs first)
    """
+    _pool: ThreadPoolExecutor


[blackfmt] _{reported by reviewdog 🐶}

Suggested change

_pool: ThreadPoolExecutor

_pool: ThreadPoolExecutor

github-actions · 2023-09-29T16:00:05Z

data_diff/thread_utils.py

 class ThreadedYielder(Iterable):
    """Yields results from multiple threads into a single iterator, ordered by priority.

    To add a source iterator, call ``submit()`` with a function that returns an iterator.
    Priority for the iterator can be provided via the keyword argument 'priority'. (higher runs first)
    """
+    _pool: ThreadPoolExecutor
+    _futures: deque
+    _yield: deque = attrs.field(alias='_yield')  # Python keyword!


[blackfmt] _{reported by reviewdog 🐶}

Suggested change

_yield: deque = attrs.field(alias='_yield') # Python keyword!

_yield: deque = attrs.field(alias="_yield") # Python keyword!

github-actions · 2023-09-29T16:00:05Z

tests/test_sql.py

        self.assertEqual(
-            "`marine_mammals`.`walrus`", self.compiler.replace(root=False).compile(table("marine_mammals", "walrus"))
+            "`marine_mammals`.`walrus`", compiler.compile(table("marine_mammals", "walrus"))
        )


[blackfmt] _{reported by reviewdog 🐶}

Suggested change

self.assertEqual(

"`marine_mammals`.`walrus`", self.compiler.replace(root=False).compile(table("marine_mammals", "walrus"))

"`marine_mammals`.`walrus`", compiler.compile(table("marine_mammals", "walrus"))

)

self.assertEqual("`marine_mammals`.`walrus`", compiler.compile(table("marine_mammals", "walrus")))

`attrs` cannot use multiple inheritance when both parents introduce their attributes (as documented). Only one side can inherit the attributes, other bases must be pure interfaces/protocols. Reimplement the `ExprNode.type` via properties to exclude it from the sight of `attrs`.

…patibility

We are going to do strict type checking. The default values of fields that clearly contradict the declared types is an error for MyPy and all other type checkers and IDEs. Remove the implicit behaviour and make nullable fields explicitly declared as such.

`attrs` is much more beneficial: * `attrs` is supported by type checkers, such as MyPy & IDEs * `attrs` is widely used and industry-proven * `attrs` is explicit in its declarations, there is no magic * `attrs` has slots But mainly for the first item — type checking by type checkers.

Since we now use `attrs` for some classes, let's use them for them all — at least those belonging to the same hierarchies. This will ensure that all classes are slotted and will strictly check that we define attributes properly, especially in cases of multiple inheritance. Except for Pydantic models and Python exceptions.

nolar · 2023-10-02T11:55:15Z

Accidentally closed by deleting the branch. Supersedes by #723.

nolar changed the base branch from master to circular-imports September 29, 2023 15:59

nolar requested a review from dlawin September 29, 2023 15:59

github-actions bot reviewed Sep 29, 2023

View reviewed changes

Sergey Vasilyev added 5 commits September 29, 2023 22:58

Convert source_table & schema to overridable properties for attrs com…

5b95ce7

…patibility

nolar force-pushed the attrs-instead-of-runtype branch from d02fad8 to c8b1989 Compare September 29, 2023 22:07

nolar mentioned this pull request Oct 2, 2023

Fix the aftermath of refactoring #722

Merged

nolar deleted the branch circular-imports October 2, 2023 11:51

nolar closed this Oct 2, 2023

nolar mentioned this pull request Oct 2, 2023

Convert to attrs, remove runtype #723

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert to `attrs`, remove `runtype` #720

Convert to `attrs`, remove `runtype` #720

nolar commented Sep 29, 2023

github-actions bot Sep 29, 2023

github-actions bot Sep 29, 2023

github-actions bot Sep 29, 2023

github-actions bot Sep 29, 2023

github-actions bot Sep 29, 2023

github-actions bot Sep 29, 2023

github-actions bot Sep 29, 2023

github-actions bot Sep 29, 2023

github-actions bot Sep 29, 2023

github-actions bot Sep 29, 2023

nolar commented Oct 2, 2023

	database_by_scheme: Dict[str, Database]

	database_by_scheme: Dict[str, Database]

	t if isinstance(t, TableAlias) else TableAlias(source_table_=t, name=parent_c.new_unique_name()) for t in elem.source_tables
	t if isinstance(t, TableAlias) else TableAlias(source_table_=t, name=parent_c.new_unique_name())
	for t in elem.source_tables

	_yield: deque = attrs.field(alias='_yield') # Python keyword!
	_yield: deque = attrs.field(alias="_yield") # Python keyword!

Convert to attrs, remove runtype #720

Convert to attrs, remove runtype #720

Conversation

nolar commented Sep 29, 2023

github-actions bot Sep 29, 2023

Choose a reason for hiding this comment

github-actions bot Sep 29, 2023

Choose a reason for hiding this comment

github-actions bot Sep 29, 2023

Choose a reason for hiding this comment

github-actions bot Sep 29, 2023

Choose a reason for hiding this comment

github-actions bot Sep 29, 2023

Choose a reason for hiding this comment

github-actions bot Sep 29, 2023

Choose a reason for hiding this comment

github-actions bot Sep 29, 2023

Choose a reason for hiding this comment

github-actions bot Sep 29, 2023

Choose a reason for hiding this comment

github-actions bot Sep 29, 2023

Choose a reason for hiding this comment

github-actions bot Sep 29, 2023

Choose a reason for hiding this comment

nolar commented Oct 2, 2023

Convert to `attrs`, remove `runtype` #720

Convert to `attrs`, remove `runtype` #720