-
Notifications
You must be signed in to change notification settings - Fork 99
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
base: master
Are you sure you want to change the base?
Conversation
"fname": orm.String(max_length=100), | ||
"lname": orm.String(max_length=100), | ||
} | ||
unique_together = (("fname", "lname"),) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Fixes #153