Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#984 closed enhancement (fixed)

ActivityIntermediator bits

Reported by: Elrond Owned by:
Priority: minor Milestone: 0.8.0
Component: programming Keywords:
Cc: Parent Tickets:

Description

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.
See http://docs.sqlalchemy.org/en/latest/orm/mapper_config.html#simple-validators

@validates('type')
def validate_type(self, key, value):
    assert value in self.TYPES
    return value

Subtickets

Change History (4)

comment:1 Changed 7 years ago by Jessica Tallon

Status: newreview

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.

comment:2 Changed 7 years ago by Elrond

That looks very good.

Little picky:

-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.

That's it.

comment:3 Changed 7 years ago by Jessica Tallon

Resolution: fixed
Status: reviewclosed

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 Changed 7 years ago by Elrond

Milestone: 0.8.0
Note: See TracTickets for help on using tickets.