Code review comment for lp:~jameinel/bzr/1.16-commit-fulltext

John A Meinel wrote:
> Andrew Bennetts wrote:
[...]
> > bzrlib/groupcompress.py:1543: undefined name 'progress'
> > bzrlib/groupcompress.py:1689: undefined name 'RevisionNotPresent'
> >
> > Now would be a good time to fix those too :)
>
> Done
>
> Though of course it would be better to have tests that provoke the
> behavior.... :)

Be my guest ;)

I'm happy to take an obvious improvement though.

[...]
> > Hmm. It would be nice if this wasn't largely the same as _add_text in
> > groupcompress.py.
>
> All but the final "self._add" versus "record = FulltextContentFactory,
> self._insert...".
>
> It didn't seem like quite enough duplication to worry about. If you
> really prefer I can refactor.

It's up to you. I can see that it would be a little awkward to refactor
(it's not obvious to me that the common logic really belongs in the
VersionedFiles base class), and I agree about not “quite enough” to require
it.

So, “it would be nice”, but that's all :)

> >> === modified file 'bzrlib/tests/test_versionedfile.py'
> > [...]
> >
> > The conditionals and repetition here are a bit ugly, but it'll do. Ideally
> > this would be done with the proper test parameterisation tools, though.
[...]
> So it is at least better, even if it isn't perfect.
>
> I suppose we could add a test permutation that tried to turn all calls
> to "add_lines" into "_add_text" and run against those. I don't feel it
> is quite worth that.

No, it probably isn't worth it. Thanks for taking a look.

-Andrew.