-
Notifications
You must be signed in to change notification settings - Fork 90
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
Composite constraints #1146
base: master
Are you sure you want to change the base?
Composite constraints #1146
Conversation
We're likely to have unique, and check constraints so I guess it makes sense to have it plural.
Thinking out loud. I like the use of columns instead of strings, but just thinking it may be worth considering getting it on the table itself somehow. I figure something like Django's Meta model which may look like the following: class Album(Table):
name = Varchar()
band = ForeignKey(Band)
class Meta:
unique_together = [Album.name, Album.band] This could also be expanded for other configs such as class Album(Table):
name = Varchar()
band = ForeignKey(Band)
class Meta:
help_text = "A music album!"
unique_together = [Album.name, Album.band] The reasoning behind the thought is just based more so on visual grouping and ensuring table related configs remain on the table itself. Note that I am aware Although ignoring the above, I like the proposed syntax and rational. I think your updated approach and method makes more sense then as table attributes. I'd be happy to use this feature in my code bases as proposed |
@Skelmis Thanks, that's great input. I agree that having it on the table class somehow is nicer. Even if there's just a |
Check constraints | ||
================= | ||
|
||
.. automethod:: Table.add_check_constraint |
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.
@dantownsend For now, constraints are droped by simply deleting (or commenting out) the constraints from the table definition. I'm fine with that, but it should be indicated in the docs so the user knows how to use that. Another option is to create a drop_unique_constraint
classmethod that will drop the constraints. As you wish, both options suit me.
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.
@dantownsend I already wrote in @atkei PR that I agree with the changes and that they are great. Using column references instead of string is a great improvement. When you finish and merge this PR it will be great for Piccolo as many people have been asking for this feature. Great.
@sinisaos Thanks. I've been thinking about it this morning. I'm leaning towards keeping both approaches - being able to define the constraints inline in the table if you're happy just using strings, and the new API if you want to use column references. Will have a think today. |
@dantownsend I think you should only use one option (one api) to create and drop unique constraints because having two ways to do the same thing could be confusing and unnecessary. That's just my opinion. You can do what you think is best. |
Based off this: #984
That PR is really great, but I've added the following:
piccolo.constraints
frompiccolo.constraint
, because we support unique and check constraints, so plural made a bit more sense to me.Remaining tasks:
In the original PR, constraints were added like this:
Which I do quite like.
In this PR I tried this:
My rational is:
Album.add_unique_constraint(Album.name, Album.band, name='unique_name_band')
. If no name is specified we generate a sensible one.We could support both syntaxes - it wouldn't be that hard, but it can be confusing having multiple ways to do the same thing.
What do people think?