Add more metadata and try to guess if relationship is iterable #74

mehdigmira · 2019-02-10T19:10:07Z

Add stuff to metadata: table name, columns, foreign keys
Try to guess is a relationship is iterable

Reset

update

ilevkivskyi · 2019-02-16T01:42:38Z

Is this ready for review, or it is still a WIP?

mehdigmira · 2019-02-16T08:23:43Z

I wanted to know first If the logic implemented is something you would consider putting into the plugin, before going any further.

mehdigmira · 2019-03-10T11:23:58Z

@ilevkivskyi This can be reviewed

mehdigmira · 2019-03-27T09:15:49Z

@ilevkivskyi Any news on this ?

ilevkivskyi · 2019-04-02T17:13:34Z

I just returned from vacation, will take a look at this later this week. Sorry for a delay!

ilevkivskyi

Thanks for the PR! Generally looks good, I have some mostly minor comments.

ilevkivskyi · 2019-07-10T14:30:07Z

sqlmypy.py

+            continue
+
+        # We currently only handle setting __tablename__ as a class attribute, and not through a property.
+        if stmt.lvalues[0].name == "__tablename__" and isinstance(stmt.rvalue, StrExpr):


Maybe we can also support constants like:

NAME: Final = 'users' class User(Base): __tablename__ = NAME ...

No need to this in this PR, but may worth leaving a TODO and/or a follow-up issue.

ilevkivskyi · 2019-07-10T14:33:25Z

sqlmypy.py

+            db_column_name = None  # type: Optional[str]
+            if 'name' in stmt.rvalue.arg_names:
+                name_str_expr = stmt.rvalue.args[stmt.rvalue.arg_names.index('name')]
+                assert isinstance(name_str_expr, StrExpr)


This is unsafe, it will crash mypy if someone passes name= as a variable or function call.

Please also add a test for this.

ilevkivskyi · 2019-07-10T14:36:06Z

sqlmypy.py

+
+            # Save foreign keys.
+            for arg in stmt.rvalue.args:
+                if (isinstance(arg, CallExpr) and isinstance(arg.callee, NameExpr)


Maybe use RefExpr instead of NameExpr?

ilevkivskyi · 2019-07-10T14:44:00Z

sqlmypy.py

+        if stmt.lvalues[0].name == "__tablename__" and isinstance(stmt.rvalue, StrExpr):
+            ctx.cls.info.metadata.setdefault('sqlalchemy', {})['table_name'] = stmt.rvalue.value
+
+        if (isinstance(stmt.rvalue, CallExpr) and isinstance(stmt.rvalue.callee, NameExpr)


I would factor out everything you add on this line and below in a helper function with a docstring explaining what we do here, e.g. process_field_assignment().

ilevkivskyi · 2019-07-10T14:45:15Z

sqlmypy.py

+                    fk = arg.args[0]
+                    if isinstance(fk, StrExpr):
+                        *r, parent_table_name, parent_db_col_name = fk.value.split(".")
+                        assert len(r) <= 1


Again, this is not safe, we should never crash on bad user input.

ilevkivskyi · 2019-07-10T14:48:07Z

sqlmypy.py

@@ -369,10 +466,18 @@ class User(Base):
            # Something complex, stay silent for now.
            new_arg = AnyType(TypeOfAny.special_form)

+    # use private api
+    current_model = ctx.api.scope.active_class()  # type: ignore # type: TypeInfo


Put # type: ignore at the end, after type comment, otherwise the type comment will be ignored.

ilevkivskyi · 2019-07-10T14:48:51Z

test/test-data/sqlalchemy-plugin-features.test

+reveal_type(child.parent)  # E: Revealed type is 'main.Parent*'
+reveal_type(parent.children)  # E: Revealed type is 'main.Child*'
+
+[out]


Please add newline at the end of file.

ilevkivskyi · 2019-07-10T14:50:00Z

test/test-data/sqlalchemy-plugin-features.test

@@ -280,3 +280,93 @@ class M2(M1):
 Base = declarative_base(cls=(M1, M2))  # E: Not able to calculate MRO for declarative base
 reveal_type(Base)  # E: Revealed type is 'Any'
 [out]
+
+[case testRelationshipIsGuessed]


Do you have tests where schema is not None?

ilevkivskyi · 2019-07-10T14:51:50Z

sqlmypy.py

+    secondaryjoin = get_argument_by_name(ctx, 'secondaryjoin')
+
+    if secondaryjoin is not None:
+        return True


Could you please add a test for this?

ilevkivskyi · 2019-07-10T14:52:48Z

test/test-data/sqlalchemy-plugin-features.test

+
+reveal_type(child.parent)  # E: Revealed type is 'main.Parent*'
+reveal_type(parent.children)  # E: Revealed type is 'typing.Iterable*[main.Child]'
+


No need for empty lines before [out].

mehdigmira · 2020-11-08T15:36:25Z

@ilevkivskyi Is this project still maintained ? Would you merge this if i make the requested changes and rebase this ?

CLAassistant · 2022-04-16T21:57:48Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ Mehdi
❌ mehdigmira

Mehdi seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

mehdigmira and others added 8 commits February 3, 2019 15:36

Merge pull request #2 from dropbox/master

7bd2ace

Reset

Merge pull request #3 from dropbox/master

6317b79

update

wip

9470f5c

better describing of table

c268644

Try to guess if a relationship is iterable

ee7baf3

mypy fixes

25c3ef3

fix

d5f2ede

add tests

605a997

mehdigmira force-pushed the add-metadata branch from 6c726f8 to 605a997 Compare February 10, 2019 19:13

explain TODO

79d0504

mehdigmira changed the title ~~[WIP] Add more metadata and try to guess if relationship is iterable~~ Add more metadata and try to guess if relationship is iterable Mar 10, 2019

keiclone mentioned this pull request Mar 22, 2019

Add views folder to output forum posts for headcount, overview, stats, dist (fixes #40) fantasy-anime-league/fantasy-anime-league#41

Merged

ilevkivskyi reviewed Jul 10, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more metadata and try to guess if relationship is iterable #74

Add more metadata and try to guess if relationship is iterable #74

mehdigmira commented Feb 10, 2019

ilevkivskyi commented Feb 16, 2019

mehdigmira commented Feb 16, 2019

mehdigmira commented Mar 10, 2019

mehdigmira commented Mar 27, 2019

ilevkivskyi commented Apr 2, 2019

ilevkivskyi left a comment

ilevkivskyi Jul 10, 2019

ilevkivskyi Jul 10, 2019

ilevkivskyi Jul 10, 2019

ilevkivskyi Jul 10, 2019

ilevkivskyi Jul 10, 2019

ilevkivskyi Jul 10, 2019

ilevkivskyi Jul 10, 2019

ilevkivskyi Jul 10, 2019

ilevkivskyi Jul 10, 2019

ilevkivskyi Jul 10, 2019

mehdigmira commented Nov 8, 2020

CLAassistant commented Apr 16, 2022


		reveal_type(child.parent) # E: Revealed type is 'main.Parent*'
		reveal_type(parent.children) # E: Revealed type is 'typing.Iterable*[main.Child]'

Add more metadata and try to guess if relationship is iterable #74

Are you sure you want to change the base?

Add more metadata and try to guess if relationship is iterable #74

Conversation

mehdigmira commented Feb 10, 2019

ilevkivskyi commented Feb 16, 2019

mehdigmira commented Feb 16, 2019

mehdigmira commented Mar 10, 2019

mehdigmira commented Mar 27, 2019

ilevkivskyi commented Apr 2, 2019

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehdigmira commented Nov 8, 2020

CLAassistant commented Apr 16, 2022