Code review comment for lp:~savilerow-team/savilerow/test-cleanup-and-py3

On Tue, Apr 8, 2014 at 11:46 AM, Chris Wayne <email address hidden> wrote:
> The dconf test wasn't actually using is_commented which made it clear to me that it wasn't necessary in the first place, and was just cluttering up the code.

Well, the code was broken. because line 46 was is_comment and line 50
it's is_commented.

But the intention of the code doesn't seem wrong to me...

What does happen if you get comments in a dconf file now? In fact, we
should add a fixture that has comments in it.

[save that for a future MP that fixes the overall dconf test as
originally mentioned...]

>
> Line 77, it would print out only information on one key, and then bail out of the test by doing self.assertTrue(False) so in that sense, the only thing that's changed is the lack of printing (which frankly I don't find that useful as the failed assertTrue will show you what is failing anyway)

Fair enough -- I read that wrong because I missed the assertTrue.

But I still think it would be more useful to try..except, and continue
in the exception case, so that the test shows all the broken keys at
once.

And that should be another test fixture -- a dconf test file with 2
broken keys, again saved for the future MP.

>
> TBH Line 123-125 was done that way just to stay under 80 chars to make pep8 not complain
>
> 146-147 again just trying to stay under 80 chars, couldn't figure out a better way to do it..

Frankly I'd just ignore pep8's advice for those particular lines
because they make the code more annoying to read.

> As for the ValueError, deleting that is an error on my part (was a little overzealous cleaning stuff up), will add it back in

Thanks.