Add more metadata and try to guess if relationship is iterable#74
Add more metadata and try to guess if relationship is iterable#74mehdigmira wants to merge 9 commits intodropbox:masterfrom
Conversation
mehdigmira
commented
Feb 10, 2019
mehdigmira
commented
- Add stuff to metadata: table name, columns, foreign keys
- Try to guess is a relationship is iterable
mehdigmira
force-pushed
the
add-metadata
branch
from
6c726f8 to
605a997
Compare
|
Is this ready for review, or it is still a WIP? |
|
I wanted to know first If the logic implemented is something you would consider putting into the plugin, before going any further. |
mehdigmira
changed the title
|
@ilevkivskyi This can be reviewed |
|
@ilevkivskyi Any news on this ? |
|
I just returned from vacation, will take a look at this later this week. Sorry for a delay! |
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks for the PR! Generally looks good, I have some mostly minor comments.
| 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): |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
This is unsafe, it will crash mypy if someone passes name= as a variable or function call.
Please also add a test for this.
|
|
||
| # Save foreign keys. | ||
| for arg in stmt.rvalue.args: | ||
| if (isinstance(arg, CallExpr) and isinstance(arg.callee, NameExpr) |
There was a problem hiding this comment.
Maybe use RefExpr instead of NameExpr?
| 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) |
There was a problem hiding this comment.
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().
| fk = arg.args[0] | ||
| if isinstance(fk, StrExpr): | ||
| *r, parent_table_name, parent_db_col_name = fk.value.split(".") | ||
| assert len(r) <= 1 |
There was a problem hiding this comment.
Again, this is not safe, we should never crash on bad user input.
| new_arg = AnyType(TypeOfAny.special_form) | ||
|
|
||
| # use private api | ||
| current_model = ctx.api.scope.active_class() # type: ignore # type: TypeInfo |
There was a problem hiding this comment.
Put # type: ignore at the end, after type comment, otherwise the type comment will be ignored.
| reveal_type(child.parent) # E: Revealed type is 'main.Parent*' | ||
| reveal_type(parent.children) # E: Revealed type is 'main.Child*' | ||
|
|
||
| [out] No newline at end of file |
There was a problem hiding this comment.
Please add newline at the end of file.
| reveal_type(Base) # E: Revealed type is 'Any' | ||
| [out] | ||
|
|
||
| [case testRelationshipIsGuessed] |
There was a problem hiding this comment.
Do you have tests where schema is not None?
| secondaryjoin = get_argument_by_name(ctx, 'secondaryjoin') | ||
|
|
||
| if secondaryjoin is not None: | ||
| return True |
There was a problem hiding this comment.
Could you please add a test for this?
|
|
||
| reveal_type(child.parent) # E: Revealed type is 'main.Parent*' | ||
| reveal_type(parent.children) # E: Revealed type is 'typing.Iterable*[main.Child]' | ||
|
|
There was a problem hiding this comment.
No need for empty lines before [out].
|
@ilevkivskyi Is this project still maintained ? Would you merge this if i make the requested changes and rebase this ? |
|
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. |
3 participants
