Skip to content

Type relationship as list #154

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

Merged
merged 3 commits into from
Jan 8, 2021
Merged

Conversation

tiangolo
Copy link
Contributor

✨ Type relationship as list.

Currently, the plugin types relationships with uselist as Iterable[X], but these relationships actually expose a list with list semantics, including .append(), assignments, etc.

This PR updates the plugin to type those relationships with List[X] instead of Iterable[X].

Ref: https://docs.sqlalchemy.org/en/13/orm/relationship_api.html#sqlalchemy.orm.relationship.params.uselist

Ref of usage with assignment: https://docs.sqlalchemy.org/en/13/orm/tutorial.html#working-with-related-objects
Ref of usage with append: https://docs.sqlalchemy.org/en/13/orm/tutorial.html#building-a-many-to-many-relationship


Note: I'm by no means expert in mypy internals and its plugins, I'm not 100% sure the usage of ctx.api.named_generic_type('builtins.list', [new_arg]) is correct here, but I see that seems to be how it's used by mypy: https://github.com/python/mypy/blob/master/mypy/plugins/ctypes.py#L158

@lukaszdudek-codibly
Copy link

Fixes #152

@lukaszdudek-codibly
Copy link

Also, this PR doesn't take into consideration #107 (not that it has to).
IMHO List should be the default type for relationship fields due to argumentation provided above by @tiangolo . Then, support for other values of collection_class can be added.

@lukaszdudek-codibly
Copy link

I've just found #82, which this PR extends by adding tests.
I guess that the topic is a pain for many people 😄

@shawnwall
Copy link
Contributor

yes, this topic is a pain. is anything else needed before this can be merged?

thanks @tiangolo for raising/fixing.

@chardigio
Copy link

Thanks for this! Just figured I'd bump @shawnwall 's comment, is anything else needed before this can be merged?

@ilevkivskyi
Copy link
Contributor

OK, I will merge this now. We can revisit then if this will cause issues with internal repos.

@ilevkivskyi ilevkivskyi merged commit c748ba3 into dropbox:master Jan 8, 2021
@tiangolo tiangolo deleted the relationship-as-list branch January 11, 2021 13:46
@tiangolo
Copy link
Contributor Author

Awesome! Thanks @ilevkivskyi ! 🚀

brabadu pushed a commit to brabadu/sqlalchemy-stubs that referenced this pull request Jan 15, 2021
Currently, the plugin types relationships with uselist as Iterable[X], but these relationships actually expose a list with list semantics, including .append(), assignments, etc.

This PR updates the plugin to type those relationships with List[X] instead of Iterable[X].

Ref: https://docs.sqlalchemy.org/en/13/orm/relationship_api.html#sqlalchemy.orm.relationship.params.uselist

Ref of usage with assignment: https://docs.sqlalchemy.org/en/13/orm/tutorial.html#working-with-related-objects
Ref of usage with append: https://docs.sqlalchemy.org/en/13/orm/tutorial.html#building-a-many-to-many-relationship

Note: I'm by no means expert in mypy internals and its plugins, I'm not 100% sure the usage of ctx.api.named_generic_type('builtins.list', [new_arg]) is correct here, but I see that seems to be how it's used by mypy: https://github.com/python/mypy/blob/master/mypy/plugins/ctypes.py#L158
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants