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

OleFileIO Upgrade #1226

Merged
merged 8 commits into from
May 29, 2015
Merged

OleFileIO Upgrade #1226

merged 8 commits into from
May 29, 2015

Conversation

radarhere
Copy link
Member

Upgrades OleFileIO to from 0.30 to the current version, 0.42b.

This series of commits, completely overwriting the existing version and then applying Pillow's local changes individually, was the cleanest way that I could think of.

@homm
Copy link
Member

homm commented May 13, 2015

Is it possible to backport all Pillow changes back to olefile?

@decalage2
Copy link

Most of the changes look ok, except a couple (4c5d4bf and 082a311): olefile should not rely on other Pillow modules (i.e. PIL._binary and _util) because it is also used as a standalone package by several other projects. This is only for a few small functions, so I would prefer to keep the code as-is for i8, i16, i32 and isPath.

Apart from that, I am ok to backport the rest upstream.

@hugovk
Copy link
Member

hugovk commented May 13, 2015

Because olefile is now installable via PyPI, should Pillow install it as a dependency instead?

It'd make version updating much simpler, but what are the other benefits and drawbacks?

@aclark4life
Copy link
Member

@hugovk Probably not, unless Pillow could not be installed without it. How about removing it completely from Pillow and documenting pip install olefile? That would be the first step toward possibly including olefile in Pillow's install_requires. Aside from being unable to read/write Microsoft OLE2 files, is there any other reason to keep OleFile code in Pillow? (Sorry to answer your questions with more questions 😄)

@hugovk hugovk added the olefile label May 14, 2015
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 74.86% when pulling dfb1248 on radarhere:olefileio_upgrade into 8f0dca2 on python-pillow:master.

@hugovk
Copy link
Member

hugovk commented May 28, 2015

@radarhere Please can you remove those things #1226 (comment) so standalone olefile isn't dependent on Pillow?

@radarhere
Copy link
Member Author

Sure thing. I've now removed those commits. However, I have also updated the methods so that Python performs the endian tasks as with the current version of _binary.

@hugovk
Copy link
Member

hugovk commented May 29, 2015

Thank you!

hugovk added a commit that referenced this pull request May 29, 2015
@hugovk hugovk merged commit af6bf89 into python-pillow:master May 29, 2015
@radarhere radarhere deleted the olefileio_upgrade branch May 29, 2015 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants