On 11/09/2016 11:48 PM, Gregory Szorc wrote:
On Wed, Nov 9, 2016 at 9:35 AM, Augie Fackler <r...@durin42.com
<mailto: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
<mailto: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'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
--
Pierre-Yves David
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
