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

Andrew Bennetts (spiv) wrote :

John A Meinel wrote:
> Andrew Bennetts wrote:
> > bzrlib/ undefined name 'progress'
> > bzrlib/ 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
> >
> 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

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

> >> === modified file 'bzrlib/tests/'
> > [...]
> >
> > 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.


« Back to merge proposal