-
Notifications
You must be signed in to change notification settings - Fork 106
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 delete cascade on DB models #608
Conversation
anitya/db/models.py
Outdated
@@ -656,7 +658,9 @@ class ProjectVersion(Base): | |||
) | |||
version = sa.Column(sa.String(50), primary_key=True) | |||
|
|||
project = sa.orm.relation('Project', backref='versions_obj') | |||
project = sa.orm.relation( |
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.
Maybe you could use the relationship API here like in here
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.
relation
is the synonym to relationship
. See here http://docs.sqlalchemy.org/en/latest/orm/relationship_api.html
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.
So why not keep consistency and use the same name either relation
or relationship
I think it would make the code easier to read.
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.
As you can see I used the same method that was already there, but you are right I didn't noticed this.
anitya/db/models.py
Outdated
@@ -679,7 +683,9 @@ class ProjectFlag(Base): | |||
updated_on = sa.Column(sa.DateTime, server_default=sa.func.now(), | |||
onupdate=sa.func.current_timestamp()) | |||
|
|||
project = sa.orm.relation('Project', backref='flags') | |||
project = sa.orm.relation( |
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.
Same as above
Codecov Report
@@ Coverage Diff @@
## master #608 +/- ##
==========================================
+ Coverage 90.54% 91.03% +0.48%
==========================================
Files 56 56
Lines 2698 2698
Branches 355 354 -1
==========================================
+ Hits 2443 2456 +13
+ Misses 197 187 -10
+ Partials 58 55 -3
Continue to review full report at Codecov.
|
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.
LGTM.
I would suggest to try to keep PRs shorter, this one was not very fun to review since it was a mixed of model change and test coverage increase. Also maybe more than 2 commits would have helped here 🙈 |
I was actually thought about separating this in two PRs, but in the end I decided to go with one PR. |
Resolved conflict. |
I decided to split this PR in two #637 to make it more readable for reviewers. The second one contains only tests and should be merged before this one to make travis pass. I also added news fragments to this PR. |
Handpicked changes from #637 so the tests could pass. |
Fixed conflicts and squashed commits. |
This should prevent orphaned DB objects. For example packages without project or distribution. Signed-off-by: Michal Konečný <[email protected]>
Fixed failing tests, forgot to start |
This should prevent orphaned DB objects.
For example packages without project or distribution.
Fixes #598