New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-34969: Add --fast, --best on the gzip CLI #9833

Merged
merged 14 commits into from Nov 3, 2018

Conversation

Projects
None yet
8 participants
@matrixise
Contributor

matrixise commented Oct 13, 2018

@JulienPalard

This comment has been minimized.

Member

JulienPalard commented Oct 13, 2018

I'm not for implementing -1 and -9 without implementing the whole range. Why not removing them, keeping only --best and --fast?

@matrixise matrixise force-pushed the matrixise:bpo-34969 branch from df7720c to 54d1131 Oct 13, 2018

@matrixise

This comment has been minimized.

Contributor

matrixise commented Oct 13, 2018

good catch, I have removed -1, -9 and -d can't accept --fast and --best.

Lib/gzip.py Outdated
parser.add_argument("-d", "--decompress", action="store_true",
group = parser.add_mutually_exclusive_group()
group.add_argument('--fast', action='store_true', help='compress faster')
group.add_argument('--best', action='store_true', help='compress bester')

This comment has been minimized.

@tirkarthi

tirkarthi Oct 13, 2018

Contributor

Minor nit, I am not sure bester is a valid word. "Best compression" maybe?

@JulienPalard

If you change the default compression from the CLI you may want to tell it in the NEWS file.

@@ -234,6 +236,23 @@ Command line options
If *file* is not specified, read from :attr:`sys.stdin`.
.. cmdoption:: --fast
Indicates the fastest compression method (less compression)

This comment has been minimized.

@JulienPalard

JulienPalard Oct 13, 2018

Member

Missing dot at end of sentence.

.. code-block:: shell-session
$ python -m gzip --best file
.. cmdoption:: -d, --decompress
Decompress the given file

This comment has been minimized.

@JulienPalard

JulienPalard Oct 13, 2018

Member

Same missing dot.

@bedevere-bot

This comment has been minimized.

bedevere-bot commented Oct 13, 2018

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

.. cmdoption:: --best
Indicates the slowest compression method (best compression).
This is the default method if you do not specify any flag.

This comment has been minimized.

@vadmium

vadmium Oct 14, 2018

Member

Out of date?

This comment has been minimized.

@matrixise

matrixise Oct 20, 2018

Contributor

@vadmium sure, if we use the level 6, it's not the --best flag.

thanks

This comment has been minimized.

@matrixise

matrixise Oct 20, 2018

Contributor

sure, thanks

.. code-block:: shell-session
$ python -m gzip --fast file

This comment has been minimized.

@vstinner

vstinner Oct 19, 2018

Member

"unversionned" python must not be used: please write python3 (and change the existing documentation for "file").

I'm not sure that it's useful to add an example of a command for each command line option.

This comment has been minimized.

@ned-deily

ned-deily Oct 20, 2018

Member

Actually, most examples in the current Doc do use an unversioned python for -m examples. Only a few currently use python3 -m. We should probably decide on which is better and fix them all. But that should be a separate issue and PR.

This comment has been minimized.

@tirkarthi

tirkarthi Oct 20, 2018

Contributor

I think it's better to use pythoninstead of python3 since that will add the extra work of replacing all python3 with python4 when we have a release in future :) Many examples will be compatible with both versions and also might cause issues in backporting doc fixes between two major versions . The URL and drop-down at the top already indicates python3.X version. I might be missing a use case here.

This comment has been minimized.

@matrixise

matrixise Oct 20, 2018

Contributor

I remove the example with the command line and in this way, just avoid the "un.versioned" python.

Lib/gzip.py Outdated
def open(filename, mode="rb", compresslevel=9,
COMPRESS_LEVEL_FAST = 1
COMPRESS_LEVEL_TRADEOFF = 6
COMPRESS_LEVEL_BEST = 9

This comment has been minimized.

@vstinner

vstinner Oct 19, 2018

Member

Please make these constants private. If you really want to make them public, you should document them.

This comment has been minimized.

@matrixise

matrixise Oct 20, 2018

Contributor

maybe with a new PR/bpo

Show resolved Hide resolved Doc/library/gzip.rst Outdated
@bedevere-bot

This comment has been minimized.

bedevere-bot commented Oct 19, 2018

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@matrixise matrixise force-pushed the matrixise:bpo-34969 branch from 841ec86 to 893da81 Oct 20, 2018

rc, out, err = assert_python_ok('-m', 'gzip', compress_level, local_testgzip)
self.assertTrue(os.path.exists(gzipname))
self.assertEqual(rc, 0)

This comment has been minimized.

@vstinner

vstinner Oct 25, 2018

Member

this test is redundant, assert_python_ok() ensures thta it's 0.

@@ -687,6 +687,19 @@ def wrapper(*args, **kwargs):
return decorator
def add_compress_level_flag(*compress_levels):

This comment has been minimized.

@vstinner

vstinner Oct 25, 2018

Member

11 lines just to pass a flag? IMHO it's too much, it can be done with less code: see below.

@@ -0,0 +1,3 @@
Add --fast, --best on the gzip CLI, these parameters will be used for the

This comment has been minimized.

@vstinner

vstinner Oct 25, 2018

Member

"gzip: Add --fast and --best options on the gzip CLI ..."

@@ -222,25 +222,27 @@ Once executed the :mod:`gzip` module keeps the input file(s).
.. versionchanged:: 3.8
Add a new command line interface with a usage.
By default, when you will execute the CLI, the default compression level is 6,
it's a good tradeoff between the best and fast compression methods.

This comment has been minimized.

@vstinner

vstinner Oct 25, 2018

Member

I suggest to remove "it's a good tradeoff between the best and fast compression methods". I wouldn't promise that it's a good tradeoff. Compression is full of bad surprises.

Just one example from gzip manual page:
"In some rare cases, the --best option gives worse compression than the default compression level (-6). On some highly redundant files, compress compresses better than gzip."

def test_compress_fast_best_are_exclusive(self):
rc, out, err = assert_python_failure('-m', 'gzip', '--fast', '--best')
self.assertIn(b"error: argument --best: not allowed with argument --fast", err)
self.assertGreater(rc, 0)

This comment has been minimized.

@vstinner

vstinner Oct 25, 2018

Member

Can't we expect a specific exit code? Same question for following test.

This comment has been minimized.

@matrixise

matrixise Oct 25, 2018

Contributor

+1

@matrixise matrixise force-pushed the matrixise:bpo-34969 branch from 893da81 to fdaa34b Oct 25, 2018

@vstinner

LGTM.

@JulienPalard: Can you please double please? You can merge it if it looks good to you.

@matrixise

This comment has been minimized.

Contributor

matrixise commented Nov 3, 2018

@JulienPalard are you interested by a merge of this PR?

@JulienPalard

This comment has been minimized.

Member

JulienPalard commented Nov 3, 2018

LGTM too :) thanks @matrixise for this PR!

@JulienPalard JulienPalard merged commit 3e28eed into python:master Nov 3, 2018

5 checks passed

Azure Pipelines PR #20181025.27 succeeded
Details
bedevere/issue-number Issue number 34969 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment