On Wed, Nov 09, 2016 at 03:48:39PM -0800, Gregory Szorc wrote: > On Wed, Nov 9, 2016 at 9:35 AM, Augie Fackler <r...@durin42.com> wrote: > > > On Mon, Nov 07, 2016 at 07:13:58PM -0800, Gregory Szorc wrote: > > > # HG changeset patch > > > # User Gregory Szorc <gregory.sz...@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 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.
If the zstd thing will need self-storage in an instance, then I'm okay to take this as-is and clean it up to be less classy if we end up not needing it. > > 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@mercurial-scm.org > > > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel > > _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel