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

# HG changeset patch
# User Gregory Szorc <gregory.szorc@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

All callers have been replaced with "compressstream." It is quite
low-level and redundant with "compressstream." So eliminate it.

On Mon, Nov 07, 2016 at 07:13:58PM -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@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!

> 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

On Wed, Nov 9, 2016 at 9:35 AM, Augie Fackler <raf@durin42.com> wrote:

> On Mon, Nov 07, 2016 at 07:13:58PM -0800, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@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@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

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@durin42.com> wrote:
>
> > On Mon, Nov 07, 2016 at 07:13:58PM -0800, Gregory Szorc wrote:
> > > # HG changeset patch
> > > # User Gregory Szorc <gregory.szorc@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
> >

On Thu, Nov 10, 2016 at 10:55:17AM -0500, Augie Fackler wrote:
> 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@durin42.com> wrote:
> >
> > > On Mon, Nov 07, 2016 at 07:13:58PM -0800, Gregory Szorc wrote:
> > > > # HG changeset patch
> > > > # User Gregory Szorc <gregory.szorc@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.

(Which is to say, queued)

On 11/09/2016 11:48 PM, Gregory Szorc wrote:
> On Wed, Nov 9, 2016 at 9:35 AM, Augie Fackler <raf@durin42.com
> <mailto:raf@durin42.com>> wrote:
>
>     On Mon, Nov 07, 2016 at 07:13:58PM -0800, Gregory Szorc wrote:
>     > # HG changeset patch
>     > # User Gregory Szorc <gregory.szorc@gmail.com <mailto:gregory.szorc@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@mercurial-scm.org
>     <mailto:Mercurial-devel@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@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>