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

Close files opened with openpyxl #282

Merged
merged 3 commits into from
Jul 13, 2021
Merged

Close files opened with openpyxl #282

merged 3 commits into from
Jul 13, 2021

Conversation

enekomartinmartinez
Copy link
Collaborator

FIles opened with pandas.read_excel are automatically closed, independently of the used engine xlrd for .xls and openpyxl for .xlsx.
However, files opened directly with openpyxl.load_workbook(.., read_only=True) are not. Therefore, a test was added and these files are now closed when Excels.clean() is called.

It was necessary to add psutil dependency for running the tests, as it is used to check which files are opened.

@coveralls
Copy link

coveralls commented Jul 12, 2021

Coverage Status

Coverage increased (+0.003%) to 98.548% when pulling d437b5b on enekomartinmartinez:close_excels into b8a6585 on JamesPHoughton:master.

@enekomartinmartinez
Copy link
Collaborator Author

@JamesPHoughton
However, I think that pandas doesn't automatically close files opened with openpyxl on Windows, see the last comment by a Windows user.
Maybe will be interesting to configure travis to run on Linux, Mac and Windows.

@pep8speaks
Copy link

pep8speaks commented Jul 13, 2021

Hello @enekomartinmartinez! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 76:80: E501 line too long (98 > 79 characters)

Comment last updated at 2021-07-13 12:21:32 UTC

@enekomartinmartinez
Copy link
Collaborator Author

@enekomartinmartinez enekomartinmartinez merged commit 36efa50 into SDXorg:master Jul 13, 2021
@enekomartinmartinez enekomartinmartinez deleted the close_excels branch July 16, 2021 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants