Skip to content
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

Use list for relationship(uselist=True) instead of iterable #82

Closed
wants to merge 1 commit into from
Closed

Use list for relationship(uselist=True) instead of iterable #82

wants to merge 1 commit into from

Conversation

rafales
Copy link
Contributor

@rafales rafales commented Jun 10, 2019

Right now relationship(uselist=True) uses typing.Iterable, which breaks most of our code.

Relationship configuration is described in greater detail here: https://docs.sqlalchemy.org/en/13/orm/collections.html

Right now this PR just replaces typing.Iterable with list.

What I plan to do in this PR:

  • default to list when using uselist=True
  • use collection_class argument to customize collection class if present (simple type)

What can be done later:

  • support dynamic loader and query_class parameters (when Typed queries #81 gets accepted)
  • support dict-like collections (attribute_mapped_collection, column_mapped_collection, mapped_collection)

@ckarnell
Copy link
Contributor

I would love this to get merged! I'm curious though, why did Iterable get used in the first place when these relationships are lists?

@rafales
Copy link
Contributor Author

rafales commented Jun 19, 2019

Probably because you can customize collection by passing collection_class argument, so Iterable was just something all possible collection classes implemented.

If @ilevkivskyi can merge this, then I'll turn from draft into regular PR and finish my work on another branch. If not yet, then I want to wait for #81 (I also need to do some fixes there) so that I can implement dynamic loader too.

@ilevkivskyi
Copy link
Contributor

This makes sense. I need to double-check this against our internal code (just in case).

There are bunch of open PRs I am going to look very soon, maybe even later this week.

@ckarnell
Copy link
Contributor

@ilevkivskyi Any update here? I've unfortunately had to add my own version of sqlmypy to our repo to get this.

@rafales
Copy link
Contributor Author

rafales commented Jul 22, 2019

I think I should at least implement collection_class before this gets merged. I'll try to do this and finish #81 this week.

@@ -372,7 +372,7 @@ class User(Base):
# We figured out, the model type. Now check if we need to wrap it in Iterable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should be updated to say list

@ilevkivskyi
Copy link
Contributor

@rafales Please let me know when this is ready for review.

@ckarnell
Copy link
Contributor

@rafales If you tell me exactly what you're thinking with the collection_class argument (maybe pseudocode), I can maybe help you get this over the finish line.

Alternatively, it seems pretty reasonable to me to merge this now, and add the collection_class customization later on.

@rafales
Copy link
Contributor Author

rafales commented Sep 18, 2019

@ckarnell I think it at least should default to list but use something else if collection_class is present (eg. Any). Support for collection_class could be improved in later PRs (supporting dicts, sets, custom classes).

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@rafales rafales closed this by deleting the head repository Nov 17, 2022
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.

None yet

5 participants