[PATCH 10 of 10 V2] util: remove compressorobj API from compression engines
Pierre-Yves David
pierre-yves.david at ens-lyon.org
Thu Nov 10 11:31:57 EST 2016
On 11/09/2016 11:48 PM, Gregory Szorc wrote:
> On Wed, Nov 9, 2016 at 9:35 AM, Augie Fackler <raf at durin42.com
> <mailto:raf at durin42.com>> wrote:
>
> On Mon, Nov 07, 2016 at 07:13:58PM -0800, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc at gmail.com <mailto:gregory.szorc at gmail.com>>
> > # Date 1478573874 28800
> > # Mon Nov 07 18:57:54 2016 -0800
> > # Node ID 6755052bb245a2fbf30a22c8b4cf5599441fabfb
> > # Parent 8e2ca8bfa2d0f7c02b6f58b36341b4c4cf7ec0f9
> > util: remove compressorobj API from compression engines
>
> I like it. Let's chat some about the interface defined in patch 1
> (class scope invites questions, and none of those instances actually
> use the self attribute so I'd like to try and get things demoted to
> module level unless there's a compelling reason), and then get this
> landed. Thanks!
>
>
> It was originally less heavyweight and Pierre-Yves's review feedback
> steered me towards the current implementation...
I'm not sure I want to bear responsibility for your V2 ;-)
My initial comment were about having:
1) more self contained class (because half of the data where in the
class and the other half were outside
2) lighter class.
Moving the name inside the class makes me happier on the (1) side, but
making everything a method is going the opposite way I was trying on (2).
It seems like most of this could be class attribute and I think this is
a change worth making.
I've the same feeling than Augie that all this could probably just
module level tuple. However I'm fine the the class approach because:
1) It seemed minor enough to not ask you to rewrite too much,
2) I suspect some of theses class to become more complex in the future.
> I agree the classes are essentially singletons and don't provide much
> value. But no matter how you slice it, you need a way to hold references
> to a collection of functions for a particular engine. That's, uh, what a
> class is for. I suppose we could pass function "pointers" to a class's
> __init__ and have it populate attributes. But the base class with stub
> methods does provide some safety and an obvious place to document the
> API. And the final result would probably have the same external API,
> just N instances of a 1 class instead of 1 instances of N classes.
>
> FWIW, I anticipate the zstd engine will need "self" storage. This is to
> facilitate C vs CFFI and lazy module importing and possibly caching of
> certain objects to facilitate faster revlog operations.
>
> I'm not opposed to rewriting the engine definition/registration code.
> But at this point I'd prefer to get *something* in place to unblock zstd
> work. We can always refine later. It's not like we have to commit to a
> BC API :)
>> All callers have been replaced with "compressstream." It is quite
>> low-level and redundant with "compressstream." So eliminate it.
>>
>> diff --git a/mercurial/util.py b/mercurial/util.py
>> --- a/mercurial/util.py
>> +++ b/mercurial/util.py
>> @@ -2966,7 +2966,7 @@ class compressionengine(object):
>> exclude the name from external usage, set the first element
> to ``None``.
>>
>> If bundle compression is supported, the class must also implement
>> - ``compressstream``, ``compressorobj`` and `decompressorreader``.
>> + ``compressstream`` and `decompressorreader``.
>> """
>> return None
>>
>> @@ -2982,14 +2982,6 @@ class compressionengine(object):
>> """
>> raise NotImplementedError()
>>
>> - def compressorobj(self):
>> - """(Temporary) Obtain an object used for compression.
>> -
>> - The returned object has ``compress(data)`` and ``flush()``
> methods.
>> - These are used to incrementally feed data chunks into a
> compressor.
>> - """
>> - raise NotImplementedError()
>> -
>> def decompressorreader(self, fh):
>> """Perform decompression on a file object.
>>
>> @@ -3006,9 +2998,6 @@ class _zlibengine(compressionengine):
>> def bundletype(self):
>> return 'gzip', 'GZ'
>>
>> - def compressorobj(self):
>> - return zlib.compressobj()
>> -
>> def compressstream(self, it, opts=None):
>> opts = opts or {}
>>
>> @@ -3039,9 +3028,6 @@ class _bz2engine(compressionengine):
>> def bundletype(self):
>> return 'bzip2', 'BZ'
>>
>> - def compressorobj(self):
>> - return bz2.BZ2Compressor()
>> -
>> def compressstream(self, it, opts=None):
>> opts = opts or {}
>> z = bz2.BZ2Compressor(opts.get('level', 9))
>> @@ -3069,7 +3055,7 @@ class _truncatedbz2engine(compressioneng
>> def bundletype(self):
>> return None, '_truncatedBZ'
>>
>> - # We don't implement compressorobj because it is hackily handled
> elsewhere.
>> + # We don't implement compressstream because it is hackily handled
> elsewhere.
>>
>> def decompressorreader(self, fh):
>> def gen():
>> @@ -3083,13 +3069,6 @@ class _truncatedbz2engine(compressioneng
>>
>> compengines.register(_truncatedbz2engine())
>>
>> -class nocompress(object):
>> - def compress(self, x):
>> - return x
>> -
>> - def flush(self):
>> - return ''
>> -
>> class _noopengine(compressionengine):
>> def name(self):
>> return 'none'
>> @@ -3097,9 +3076,6 @@ class _noopengine(compressionengine):
>> def bundletype(self):
>> return 'none', 'UN'
>>
>> - def compressorobj(self):
>> - return nocompress()
>> -
>> def compressstream(self, it, opts=None):
>> return it
>>
>
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel at mercurial-scm.org
> <mailto:Mercurial-devel at mercurial-scm.org>
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> <https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel>
>
>
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
--
Pierre-Yves David
More information about the Mercurial-devel
mailing list