launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

The proposal to merge lp:~jtv/maas/timestamped-model into lp:maas has been updated.

Description changed to:

Remember CommonInfo?  It's a base class used for some of our models.  Its purpose is not very clear, which will probably lead to either complications or ossification.

And so in this branch I break it up into separate ways to address its two purposes, as mentioned in today's standup and also in previous private chat with Julian:

 1. Add timestamps to certain models.  This becomes a new, single-purpose class.

 2. Always check object validity before saving.  This is now a method on the model itself.

That also means that we can decouple the two.  I'm not sure integrating the validity check with save() is the best thing to do, but nor do I see any other clear “line of defense” where these things should be checked.  Somebody with more Django experience (that means you Raphaël) will surely be able to help.

The custom skip_check parameter to save() is gone.  This kind of parameter is usually a symptom that too much functionality is being heaped into a single function: if there's a part you don't always need, it's usually best off as a separate call.  In this case, the parameter was really only there so that the factory can create nodes in any initial state, rather than just the ones that we allow in production.  And so I had the factory temporarily sabotage that specific check at just that one critical point.

Jeroen

For more details, see:
https://code.launchpad.net/~jtv/maas/timestamped-model/+merge/104503
-- 
https://code.launchpad.net/~jtv/maas/timestamped-model/+merge/104503
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/timestamped-model into lp:maas.