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

Add support for unique_together #154

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SepehrBazyar
Copy link

Fixes #153

"fname": orm.String(max_length=100),
"lname": orm.String(max_length=100),
}
unique_together = (("fname", "lname"),)
Copy link
Member

Choose a reason for hiding this comment

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

Hey @SepehrBazyar ,

Thank you for the PR. I haven't really given this much thought but what do you think about this style?

class Customer(orm.Model):
    registry = ...
    fields = ...
    constraints = (
        orm.UniqueConstraint("name"),
         ...
     )

And the constraints are just shortcuts to the SQLAlchemy constraints.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @aminalaee
Thank you for your attention.

This style is completely OK but I think we should pay attention to what we want and what matters to us; then choose accordingly.
So, if we want it to be more user-friendly and easier to employ, then my style would probably come in handy because it's more similar to Django.

But if we want it to be more raw and similar to SQLAlchemy constraints, then your style would be a perfect choice.
But there is one problem I can think of!
In here, we don't have a class Meta, thus all and all of our attributes will be defined under class Model! So if we want unique constraints, check constraints and etcetera in one attribute, this field could get crowded.

At last, it's better to support both styles and have it all!

Copy link
Member

Choose a reason for hiding this comment

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

yes I think so. Let's wait for more feedback and see what other people think.

Copy link

Choose a reason for hiding this comment

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

IMHO, I think we can follow a way as long as it is documented.

With that, perhaps the SQLAlchemy approach is better since it is known that ORM uses the SQLAlchemy core.

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.

How to support 'unique_together'
3 participants