bpo-32682: test_zlib improve version parsing by pmp-p · Pull Request #5347

bpo-32682: test_zlib improve version parsing by pmp-p · Pull Request #5347 · python/cpython

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

pmp-p changed the title ~~issue 32682: test_zlib improve version parsing~~ # 32682: test_zlib improve version parsing

Jan 26, 2018

		# wbits=0 only supported since zlib v1.2.3.5
		# Register "1.2.3" as "1.2.3.0"
		v = (zlib.ZLIB_RUNTIME_VERSION + ".0").split(".", 4)
		if zlib.ZLIB_RUNTIME_VERSION.find('-')>=0:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"-" in zlib.ZLIB_RUNTIME_VERSION would be simpler

		v = list(map( int , zlib.ZLIB_RUNTIME_VERSION.split('-',1)[0].split('.')[:3] ))
		v.append(0)
		else:
		v = (zlib.ZLIB_RUNTIME_VERSION + ".0").split(".", 4)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should factorize the code between the if/else block. Something like:

v = zlib.ZLIB_RUNTIME_VERSION
if "-" in v:
v = v.split('-',1)[0]
v = (v + ".0").split(".", 4)

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.

pmp-p changed the title ~~# 32682: test_zlib improve version parsing~~ bpo-32682: test_zlib improve version parsing

Jan 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this way

or this way ? ( sorry learning interface :p )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a line too long

I have made the requested changes; please review again

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of minor fixes needed.

		supports_wbits_0 = ( v[0] > 1 ) or ( v[0] == 1 ) \
		and ( v[1] > 2 ) or ( v[1] == 2 ) \
		and ( v[2] > 3 ) or ( v[2] == 3 ) \
		and ( v[3] >= 5 )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not supports_wbits_0 = v >= (1, 2, 3, 5)?

		elif not v[-1].isnumeric():
		v[-1] = '0'

		v = tuple( map( int, v ) )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for the extra spaces around ( or ); v = tuple(map(int, v)). (See PEP 8.)

		and ( v[1] > 2 ) or ( v[1] == 2 ) \
		and ( v[2] > 3 ) or ( v[2] == 3 ) \
		and ( v[3] >= 5 )
		del v

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for del v.

I have made the requested changes; please review again

Thanks for making the requested changes!

@vstinner, @zware: please review the changes made to this pull request.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Thanks @pmp-p for the PR, and @zware for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Feb 19, 2018

(cherry picked from commit 4c7108a)

Co-authored-by: pmp-p <pmp-p@users.noreply.github.com>

Sorry, @pmp-p and @zware, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 4c7108a77144493d0aa6fc0105b67d3797e143f5 2.7

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Feb 19, 2018

(cherry picked from commit 4c7108a)

Co-authored-by: pmp-p <pmp-p@users.noreply.github.com>

@serhiy-storchaka This doesn't appear to need to go back to 2.7; would you mind taking care of that one way or the other? I would vote for the simple solution of just removing the label :)

miss-islington added a commit that referenced this pull request

Feb 19, 2018

(cherry picked from commit 4c7108a)

Co-authored-by: pmp-p <pmp-p@users.noreply.github.com>

miss-islington added a commit that referenced this pull request

Feb 19, 2018

(cherry picked from commit 4c7108a)

Co-authored-by: pmp-p <pmp-p@users.noreply.github.com>

pmp-p mannequin mentioned this pull request

Apr 10, 2022