Code review comment for lp:~nuclearbob/utah/docstring-cleanup

Revision history for this message
Max Brustkern (nuclearbob) wrote :

> On 04/08/2013 11:03 AM, Max Brustkern wrote:
> > I removed the MissingData check because we said in the code review
> > that we wanted to remove that since we're using the validator now.
> > The validator code that's in dev right now (and I think production)
> > is already broken, and I don't think removing that check will make
> > things worse.
>
> If you remove this, then we need to assign those attributes default
> values in the schema so that they get set. Otherwise, I think a "bad"
> control file could cause us to try and reference variables that haven't
> been initialized.
>
> > We can leave it in until that's fixed if you want, but
> > either way, we should fix it soon, since we're currently behaving in
> > confusing and bad ways when you try to run a bad tc_control file.
>
> I would like to fix it soon. Just not sure we want to fit it in this.

My feeling is, this is already broken. I don't see a MissingData exception get raised when I feed in a file with bad data, even without that change in place, so I don't think that code is doing anything useful. Maybe that means we should leave it in until the branch that fixes it (which I'm hoping to propose today or tomorrow.) As I'm typing this, that actually makes more sense. If nobody disagrees, I can put that block and the test back in. That will mean the test is failing, but the correction for that is something we're going to want as a bugfix anyway, so landing it separately makes sense.

« Back to merge proposal