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.

Sign up for GitHub

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

Jump to bottom

Make OleFileIO VT dict building more comprehensive #1139

Closed
wants to merge 1 commit into from
Closed

Make OleFileIO VT dict building more comprehensive #1139

wants to merge 1 commit into from

Conversation

vperron
Copy link

@vperron vperron commented Mar 20, 2015

Mainly a style fix.

@vperron
Make OleFileIO VT dict building more comprehensive
321839e 
Also is more resilient to a weird RuntimeError upon
iteration sometimes yielded in tests.
@decalage2
Copy link

Looks good, I will test it and merge it upstream into olefile (http://www.decalage.info/olefile)

@decalage2
Copy link

I would be interested in details about the RuntimeError you mentioned, if there is any issue I need to fix in olefile.

@vperron
Copy link
Author

vperron commented Mar 20, 2015

Well the runtime error did happen in a non-easily reproducible way and was about the vars() dictionary being modified upon iteration.

That's why I refer to it as a style fix.

@hugovk
Copy link
Member

hugovk commented Mar 21, 2015

@decalage2 Let us know if you merge it upstream as is, and we can just hit the merge button here.

@aclark4life aclark4life added this to the 2.8.0 milestone Mar 26, 2015
@aclark4life
Copy link
Member

@decalage2 Any progress on this?

@aclark4life aclark4life modified the milestones: 2.9.0, 2.8.0 Apr 1, 2015
@hugovk hugovk added the olefile label May 10, 2015
@radarhere radarhere modified the milestones: 3.0.0, 2.9.0 Jul 14, 2015
@radarhere
Copy link
Member

Given that it's been a while, and that substantial changes to OleFileIO have already been merged with #1226, is it time to merge this?

@aclark4life
Copy link
Member

I think we need to hear from @decalage2 and if we don't, decide if we want to merge anyway… or not. I'm not exactly sure what's appropriate or needed.

@wiredfool wiredfool removed this from the 3.0.0 milestone Sep 20, 2015
@jdufresne jdufresne mentioned this pull request Nov 5, 2016
Merged
@radarhere
Copy link
Member

With the merging of #2199, this is no longer applicable.

@radarhere radarhere closed this Dec 18, 2016
@decalage2 decalage2 mentioned this pull request Dec 18, 2016
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Do Not Merge Enhancement olefile
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vperron @decalage2 @hugovk @aclark4life @radarhere @wiredfool