[PATCH 10 of 10 V2] util: remove compressorobj API from compression engines

[Augie Fackler]

On Wed, Nov 09, 2016 at 03:48:39PM -0800, Gregory Szorc wrote:
> On Wed, Nov 9, 2016 at 9:35 AM, Augie Fackler <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>
> > > # 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 at mercurial-scm.org
> > > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >