[PATCH] Tests for KnitContent class

[Dmitry Vasiliev]

John Arbash Meinel wrote:
> Dmitry Vasiliev wrote:
>> Actually the KnitContent's interface looks incomplete for me because of
>> so many places where KnitContent._lines is directly accessed.
> 
> Is this internal to KnitContent, or external? Meaning is KnitIndex
> accessing KnitContent._lines?

External, I don't see any problem if an object uses its attributes internally 
except the case when it duplicates a code at some point.

> I don't think there would be a problem with either:
> 
> 1) Changing places that use it to use KnitContent.annotate_iter
> 
> 2) Providing an 'def __iter__()' so that we could just use:
> 
>   for origin, line in content:

Actually there's even worse cases of external changing and replacing 
KnitContent._lines.

> I think the KnitContent interface is as it is, because of trying to look
> like the VersionedFile api, rather than just being what it is, which is
> a private Knit helper class.
> KnitVersionedFile needs to maintain the interface, but KnitContent doesn't.

I see the problem as follows: at some point the KnitContent clients just 
started to abuse the KnitContent interface instead of updating it and now we've 
got a bad smelled code. The only way which follow to more clean and more 
maintainable code is extraction some code from the clients into some of 
KnitContent methods.

> I realize KnitContent isn't technically private (because it doesn't have
> a leading '_'.) But I think it was logically meant to be private. So I
> personally would approve making it private. And if we really wanted we
> could have a deprecated
> 
> @deprecated(zero_fourteen)
> def KnitContent(*args, **kwargs):
>   """KnitContent is supposed to be private."""
>   return _KnitContent(*args, **kwargs)

I don't think any name changes are needed, but in either case deprecation 
shouldn't mutate a class into a function. I guess subclassing and deprecation 
of the __init__ should work just well.

-- 
Dmitry Vasiliev (dima at hlabs.spb.ru)
     http://hlabs.spb.ru