Skip to content
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

More Python 3 fixes #25

Merged
merged 6 commits into from
Sep 15, 2018
Merged

More Python 3 fixes #25

merged 6 commits into from
Sep 15, 2018

Conversation

ale-rt
Copy link
Member

@ale-rt ale-rt commented May 12, 2018

No description provided.

@ale-rt ale-rt requested review from davisagli and pbauer May 12, 2018 08:17
@ale-rt
Copy link
Member Author

ale-rt commented May 12, 2018

There is already a relevant changelog line

)
zf.close()
zf.writestr(
'/'.join([self.__name__, path, ]),
Copy link
Member

Choose a reason for hiding this comment

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

The zipfile docs say: "There is no official file name encoding for ZIP files. If you have unicode file names, you must convert them to byte strings in your desired encoding before passing them to write()."

Since directory listings are strings in Python 3 I think that means we need to encode the names for zipfile in Python 3 -- but I haven't actually tested.

Copy link
Member Author

Choose a reason for hiding this comment

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

@davisagli thanks for your precious review!

It seems tests are running fine.

[ale@emily plone3]$ .tox/py36/bin/test -s plone.resource 
...
Total: 79 tests, 0 failures, 0 errors, 0 skipped in 8.572 seconds.

Indeed I do not think in the tests we have paths with fancy unicode characters.

Also that note is for the filename parameter of the write method: https://docs.python.org/3/library/zipfile.html#zipfile.ZipFile.write
On the other way some lines after we read: "zinfo_or_arcname is either the file name it will be given in the archive, or a ZipInfo instance."

I set this one in progress and will add a new test to sort this out!

@pbauer
Copy link
Member

pbauer commented Sep 14, 2018

This will only work in Plone 5.2 (due to changes in OFS.Image in Zope 4), so we'll need a legacy-branch for Plone 5.0 and 5.1.

@pbauer
Copy link
Member

pbauer commented Sep 14, 2018

branch 2.0.x is for 5.0 and 5.1, branch master (next release will be 2.1.0) for 5.1
Done with plone/buildout.coredev@a1c8070 and plone/buildout.coredev@284608a

@pbauer pbauer merged commit e766c8b into master Sep 15, 2018
@pbauer pbauer deleted the python3 branch October 3, 2018 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants