Movatterモバイル変換

Lib/zipimport.py Outdated


		path = _get_module_path(self, fullname)
		if mi:
		fullpath = path + path_sep + '__init__.py'

Copy link

Member

Choose a reason for hiding this comment

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

bootstrap_external._path_join()

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

What is the benefit of usingbootstrap_external._path_join()?

Copy link

Member

Choose a reason for hiding this comment

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

Readability. When I readpath + path_sep + '__init__.py' I have to mentally piece together that this is constructing a file path, while_path_join(path, '__init__.py)` gives me that context implicitly.

Lib/zipimport.py Outdated

		if mi:
		fullpath = path + path_sep + '__init__.py'
		else:
		fullpath = path + '.py'

Copy link

Member

brettcannonAug 17, 2018

Choose a reason for hiding this comment

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

Might as well use an f-string.

Copy link

MemberAuthor

serhiy-storchakaAug 17, 2018

Choose a reason for hiding this comment

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

What is the benefit of using an f-string instead of+ for concatenating two strings?

Copy link

Member

brettcannonAug 24, 2018

Choose a reason for hiding this comment

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

I find it more readable.

Lib/zipimport.py Outdated

		# add __path__ to the module before the code gets
		# executed
		path = _get_module_path(self, fullname)
		fullpath = f'{self.archive}{path_sep}{path}'

Copy link

Member

brettcannonAug 17, 2018

Choose a reason for hiding this comment

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

_bootstrap_external._path_join()

Lib/zipimport.py Outdated


		# implementation

		def _unpack_uint32(data):

Copy link

Member

brettcannonAug 17, 2018

Choose a reason for hiding this comment

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

_bootstrap_external._r_long()

Copy link

MemberAuthor

serhiy-storchakaAug 17, 2018

Choose a reason for hiding this comment

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

What to do with_unpack_uint16?_unpack_uint32 looks to me more descriptive than_r_long.

Copy link

Member

brettcannonAug 24, 2018•
edited
Loading

Choose a reason for hiding this comment

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

You can keep_unpack_uint16, I just don't think we need to have two implementations to do the exact same thing of reading in the bytes of a little-endian 4 bytes int. If you don't like the name then feel free to rename it in importlib.

Lib/zipimport.py Outdated

		name = name.decode('latin1').translate(cp437_table)

		name = name.replace('/', path_sep)
		path = f'{archive}{path_sep}{name}'

Copy link

Member

brettcannonAug 17, 2018

Choose a reason for hiding this comment

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

bootstrap_external._path_join()

brettcannon requested a review fromwarsaw

August 17, 2018 18:03

serhiy-storchaka added3 commits

August 17, 2018 22:29

Merge branch 'master' into zipimport

d34d754

Try to fix a build on Windows.

a683768

Fix tests on Windows.

3928725

berkerpeksag reviewed

Aug 19, 2018

Lib/zipimport.pyShow resolvedHide resolved

Lib/zipimport.py Outdated


		def _unpack_uint16(data):
		"""Convert 2 bytes in little-endian to an integer."""
		assert len(data) == 2

Copy link

Member

berkerpeksagAug 19, 2018

Choose a reason for hiding this comment

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

Nit: Wouldn't be better if we raisedValueError here and in_unpack_uint32()?

Copy link

MemberAuthor

serhiy-storchakaAug 19, 2018

Choose a reason for hiding this comment

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

In all cases the length of the data is checked before calling_unpack_uint*(). The assertion condition is always true.

serhiy-storchaka added2 commits

August 25, 2018 09:21

Merge branch 'master' into zipimport

3ea8986

Address Brett's comments.

6a78b4c

brettcannon approved these changes

Aug 31, 2018

warsaw reviewed

Copy link

Member

warsaw left a comment

Choose a reason for hiding this comment

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

Comments just based on reading the code. I'm going to test this branch with importlib_resources (standalone) next.

Lib/test/test_bdb.pyShow resolvedHide resolved

Lib/importlib/_bootstrap_external.pyShow resolvedHide resolved

Lib/test/test_zipimport.pyShow resolvedHide resolved

Lib/zipimport.py Outdated

		@@ -0,0 +1,640 @@
		'''zipimport provides support for importing Python modules from Zip archives.

Copy link

Member

warsawSep 11, 2018

Choose a reason for hiding this comment

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

Style nit: this should be a""" string.

Copy link

MemberAuthor

serhiy-storchakaSep 11, 2018

Choose a reason for hiding this comment

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

Done.

Lib/zipimport.pyShow resolvedHide resolved

Lib/zipimport.py Outdated

		try:
		raw_data = fp.read(data_size)
		except OSError:
		raise OSError("zipimport: can't read data")

Copy link

Member

warsawSep 11, 2018

Choose a reason for hiding this comment

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

Is this just here to change the error message? I guess that's for compatibility with the message raised by the C implementation? I'm not sure how useful that is, and it does kind of hide the original exception (which might be useful for debugging). My inclination is to preserve the originalOSError rather than produce one with a different message, but if you feel otherwise, why not use araise from here too?

Copy link

MemberAuthor

serhiy-storchakaSep 11, 2018

Choose a reason for hiding this comment

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

Removed the try block.

Lib/zipimport.py Outdated

		try:
		decompress = _get_decompress_func()
		except:
		raise ZipImportError("can't decompress data; zlib not available")

Copy link

Member

warsawSep 11, 2018

Choose a reason for hiding this comment

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

except Exception andraise from

Copy link

MemberAuthor

serhiy-storchakaSep 11, 2018

Choose a reason for hiding this comment

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

Addedexcept Exception.

Lib/zipimport.pyShow resolvedHide resolved

warsaw requested changes

Copy link

Member

warsaw left a comment

Choose a reason for hiding this comment

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

The branch does work with importlib_resources AFAICT.

Copy link

bedevere-bot commentedSep 11, 2018

When you're done making the requested changes, leave the comment:I have made the requested changes; please review again.

bedevere-bot added awaiting changes and removed awaiting merge labels

Merge branch 'master' into zipimport

14c8601

python deleted a comment fromwarsaw

Addressed commit reviews.

5fed013

serhiy-storchaka commented

Copy link

MemberAuthor

serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Thank you for your review@warsaw. I have addressed most of your comments. But AFAIKfrom error is not needed in the Python code. Exceptions is chained by default. You need to addfrom None for suppressing chaining.

Lib/importlib/_bootstrap_external.pyShow resolvedHide resolved

Lib/test/test_bdb.pyShow resolvedHide resolved

Lib/test/test_zipimport.pyShow resolvedHide resolved

Lib/zipimport.py Outdated

		'PQRSTUVWXYZ[\\]^_'
		'`abcdefghijklmno'
		'pqrstuvwxyz{\|}~\x7f'

Copy link

MemberAuthor

serhiy-storchakaSep 11, 2018

Choose a reason for hiding this comment

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

Just for readability. The above part is ASCII, and 16 chars per row, the below part is non-ASCII, and 8 chars per row. What comment should I add?

Lib/zipimport.py Outdated

		from zlib import decompress
		except:
		_bootstrap._verbose_message('zipimport: zlib UNAVAILABLE')
		raise ZipImportError("can't decompress data; zlib not available")

Copy link

MemberAuthor

serhiy-storchakaSep 11, 2018

Choose a reason for hiding this comment

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

The C implementation catches all exceptions. I agree, that this is evil and will change this.

Lib/zipimport.py Outdated

		try:
		raw_data = fp.read(data_size)
		except OSError:
		raise OSError("zipimport: can't read data")

Copy link

MemberAuthor

serhiy-storchakaSep 11, 2018

Choose a reason for hiding this comment

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

Removed the try block.

Lib/zipimport.py Outdated

		try:
		decompress = _get_decompress_func()
		except:
		raise ZipImportError("can't decompress data; zlib not available")

Copy link

MemberAuthor

serhiy-storchakaSep 11, 2018

Choose a reason for hiding this comment

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

Addedexcept Exception.

Lib/zipimport.pyShow resolvedHide resolved

serhiy-storchaka added2 commits

September 17, 2018 13:53

Merge branch 'master' into zipimport

3984d25

Add additional comments for the cp437 table.

33fa5a4

serhiy-storchaka commented

Sep 17, 2018

Copy link

MemberAuthor

serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

I prefer to make behavior changing clean up and remove unneeded tests in separate PR.

Lib/zipimport.pyShow resolvedHide resolved

Lib/zipimport.py

		try:
		toc_entry = self._files[key]
		except KeyError:
		raise OSError(0, '', key)

Copy link

MemberAuthor

serhiy-storchakaSep 17, 2018

Choose a reason for hiding this comment

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

For raising a properFileNotFoundError you need to specify the corresponding errno.

importerrnoraiseFileNotFoundError(errno.ENOENT,'No such file or directory',key)

(and maybe usingpathname is more proper thankey).

I prefer to defer this change to later. It is nontrivial and may need additional discussion and tests.

warsaw requested changes

Sep 17, 2018

Copy link

Member

warsaw left a comment

Choose a reason for hiding this comment

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

I think we're down to one issue/question for this branch, a merge conflict, and deferring cleanups to a follow on branch.

Merge branch 'master' into zipimport

ec110a0

warsaw approved these changes

Copy link

Member

warsaw left a comment

Choose a reason for hiding this comment

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

Thanks for doing this - I think it will lead to great improvements in zipimport in the future. My only suggestion would be to open a bpo issue for the subsequent clean up branch, since there are things that we know can be improved on the next pass. I think this is a good change to land.

bedevere-bot added awaiting merge and removed awaiting changes labels

serhiy-storchaka merged commit79d1c2e intopython:master

bedevere-bot removed the awaiting merge label