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

[Gregory Szorc]

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.

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20161109/188d5c26/attachment.html>