Opened 9 years ago
Closed 9 years ago
Last modified 9 years ago
#984 closed enhancement (fixed)
|Reported by:||Elrond||Owned by:|
While looking at ActivityIntermediator, I noticed some bits.
The .set method could probably be rewritten a bit:
# We need to flush so that self.id is populated self.type = key Session.flush() # First set self as activity obj.activity = self.id Session.commit()
And instead of checking values in
.save use a validator.
@validates('type') def validate_type(self, key, value): assert value in self.TYPES return value
Change History (4)
comment:1 by , 9 years ago
|Status:||new → review|
comment:2 by , 9 years ago
That looks very good.
-from mediagoblin.db.base import Base, DictReadAttrProxy +from mediagoblin.db.base import Base, DictReadAttrProxy, Session
Is this needed, now that we have commit=False?
- # We need to save so that self.id is populated
I like this comment, because it explains, why the .save() the save and commit=False is needed.
comment:3 by , 9 years ago
|Status:||review → closed|
I have removed the unneeded Session import and reintroduced a comment to explain why I'm doing a
self.save(commit=False). This has been merged and pushed to master, the commit is 44c53d3.
comment:4 by , 9 years ago
I've made the changes you've said above and also added some unit tests around the getters and setters for Activity/ActivityIntermediator. The commit is af53fa4.
Let me know when you've checked that there is nothing else so I can merge this.