Code review comment for lp:~allenap/launchpad/job-status-validator

Revision history for this message
Aaron Bentley (abentley) wrote :

I didn't make _status private because only some transitions are valid. That would be silly, because you can just make a read/write property for that. And the solution you've given would be even better in that case.

I made status private because status is part of a more complex state that includes the date_started and date_finished. It should not be set in isolation, so I made it private and provided methods for setting it to the desired state. By providing a writable status, you encourage people to set it directly, which will produce inconsistent states (e.g. Job.status == JobStatus.RUNNING, but Job.date_started is None).

For example, prepare_job will apparently produce inconsistent states. IIUC, prepare_job(JobStatus.RUNNING, JobStatus.COMPLETED) will produce a job that is COMPLETED, but has no date_started or date_finished. Now obviously, this is test code and it doesn't matter for those particular tests, but at least in the previous version, it was clear that they were doing something slightly dirty, because they were setting a private variable.

review: Disapprove

« Back to merge proposal