| Submitter | Gregory Szorc |
|---|---|
| Date | Nov. 2, 2016, 12:08 a.m. |
| Message ID | <60f180c9a030ebcee6c6.1478045314@ubuntu-vm-main> |
| Download | mbox | patch |
| Permalink | /patch/17244/ |
| State | Accepted |
| Headers | show |
Comments
On 11/02/2016 01:08 AM, Gregory Szorc wrote: > # HG changeset patch > # User Gregory Szorc <gregory.szorc@gmail.com> > # Date 1476577441 25200 > # Sat Oct 15 17:24:01 2016 -0700 > # Node ID 60f180c9a030ebcee6c6f4f8584fdb94c73ac337 > # Parent e5cc44ea12de681d971fcbebb65a7fb71fd1c3c7 > util: put compression code next to each other > > ctxmanager was injecting itself between the compression and > decompression code. Let's restore some order. patch 1 is pushed, patch 2 is yet to be reviewed. Some feedback on your submition scheme, given the size of patches 2 (and therefor likeness of comment), it would have been better to keep the series small in the form of: 1) preparatory patches 2) large/new/complex patch 3) minimal usage of the new code Then you can send a larger series of more trivial usage of the new code once it is accepted.
On Wed, Nov 2, 2016 at 7:26 AM, Pierre-Yves David < pierre-yves.david@ens-lyon.org> wrote: > > > On 11/02/2016 01:08 AM, Gregory Szorc wrote: > >> # HG changeset patch >> # User Gregory Szorc <gregory.szorc@gmail.com> >> # Date 1476577441 25200 >> # Sat Oct 15 17:24:01 2016 -0700 >> # Node ID 60f180c9a030ebcee6c6f4f8584fdb94c73ac337 >> # Parent e5cc44ea12de681d971fcbebb65a7fb71fd1c3c7 >> util: put compression code next to each other >> >> ctxmanager was injecting itself between the compression and >> decompression code. Let's restore some order. >> > > patch 1 is pushed, patch 2 is yet to be reviewed. > > Some feedback on your submition scheme, given the size of patches 2 (and > therefor likeness of comment), it would have been better to keep the series > small in the form of: > > 1) preparatory patches > 2) large/new/complex patch > 3) minimal usage of the new code > > Then you can send a larger series of more trivial usage of the new code > once it is accepted. From my experience, submitting code to introduce a new API without clear examples of how that API is used makes things harder because it isn't clear how the new code will be used and/or why it is necessary. A reviewer could justifiably drag their feet until these questions are answered. Code answers those questions. I agree that part 2 could have been split. However, I thought it best to introduce a semi-complete snapshot of the API in a single go so reviewers didn't have to piece it together from several diffs. Now that you've seen the general scope of what I'm doing, if you want me to split it up, I can.
Patch
diff --git a/mercurial/util.py b/mercurial/util.py --- a/mercurial/util.py +++ b/mercurial/util.py @@ -2789,42 +2789,16 @@ if safehasattr(parsers, 'dirs'): dirs = parsers.dirs def finddirs(path): pos = path.rfind('/') while pos != -1: yield path[:pos] pos = path.rfind('/', 0, pos) -# compression utility - -class nocompress(object): - def compress(self, x): - return x - def flush(self): - return "" - -compressors = { - None: nocompress, - # lambda to prevent early import - 'BZ': lambda: bz2.BZ2Compressor(), - 'GZ': lambda: zlib.compressobj(), - } -# also support the old form by courtesies -compressors['UN'] = compressors[None] - -def _makedecompressor(decompcls): - def generator(f): - d = decompcls() - for chunk in filechunkiter(f): - yield d.decompress(chunk) - def func(fh): - return chunkbuffer(generator(fh)) - return func - class ctxmanager(object): '''A context manager for use in 'with' blocks to allow multiple contexts to be entered at once. This is both safer and more flexible than contextlib.nested. Once Mercurial supports Python 2.7+, this will become mostly unnecessary. ''' @@ -2875,16 +2849,42 @@ class ctxmanager(object): except BaseException: pending = sys.exc_info() exc_type, exc_val, exc_tb = pending = sys.exc_info() del self._atexit if pending: raise exc_val return received and suppressed +# compression utility + +class nocompress(object): + def compress(self, x): + return x + def flush(self): + return "" + +compressors = { + None: nocompress, + # lambda to prevent early import + 'BZ': lambda: bz2.BZ2Compressor(), + 'GZ': lambda: zlib.compressobj(), + } +# also support the old form by courtesies +compressors['UN'] = compressors[None] + +def _makedecompressor(decompcls): + def generator(f): + d = decompcls() + for chunk in filechunkiter(f): + yield d.decompress(chunk) + def func(fh): + return chunkbuffer(generator(fh)) + return func + def _bz2(): d = bz2.BZ2Decompressor() # Bzip2 stream start with BZ, but we stripped it. # we put it back for good measure. d.decompress('BZ') return d decompressors = {None: lambda fh: fh,