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

Issue #469 is fixed (When run on Windows a lot of tests fail with the error: [Errno 13] Permission denied) #484

Merged
merged 9 commits into from
Oct 26, 2020

Conversation

Ev2geny
Copy link

@Ev2geny Ev2geny commented Aug 31, 2020

Pull request
This fixes issue
When run on Windows a lot of tests fail with the error: [Errno 13] Permission denied #469

Both detailed problem analysis as well as the solution implemented are described in #469

How Has This Been Tested?
I ran tests in Windows 10 environment and they all pass now

Checklist

  • I have added tests that prove my fix is effective or that my feature
    works
  • I have added docstrings to newly created methods and classes - n/a
  • I have optimized the code at least one time after creating the initial
    version -
  • I have updated the README.md or I am verified that this
    is not necessary
  • I have updated the readthedocs documentation or I
    verified that this is not necessary
  • I have added a consice human-readable description of the change to
    CHANGELOG.md

@pietermarsman
Copy link
Member

Hi @Ev2geny, good catch! I've left a comment on the issue with some comments on the proposed solution. Could you change the PR to reflect that?

What I'm suggesting is to create a new context manager TemporaryFilePath() that returns a temporary file path (without creating it) and to delete it after it is used.

If you don't know context managers (that's what the with thing is) you could read this.

@Ev2geny
Copy link
Author

Ev2geny commented Sep 14, 2020

@pietermarsman , I think this is logical to do. I will re-write PR with context manager TemporaryFilePath()

@Ev2geny
Copy link
Author

Ev2geny commented Sep 18, 2020

I have implemented context manager. All tests pass, except the ones on Python 3.4.
Since support for 3.4 is already decided to be dropped for pdf miner (#503) I assume I assume I can just wait for (#503) to be fixed

@pietermarsman
Copy link
Member

Yep, lets wait for #503. The error is caused because prefix and suffix are directly added to the filename. I guess the default values are different in python 3.4 (e.g. empty string).

@pietermarsman
Copy link
Member

I removed python 3.4 from the following release. Could you rebase on develop and force push?

@Ev2geny
Copy link
Author

Ev2geny commented Oct 25, 2020

I removed python 3.4 from the following release. Could you rebase on develop and force push?
@pietermarsman , this is done now

@pietermarsman pietermarsman merged commit 693e4f4 into pdfminer:develop Oct 26, 2020
@pietermarsman
Copy link
Member

Thanks!

jstockwin pushed a commit that referenced this pull request Oct 26, 2020
… error: [Errno 13] Permission denied) (#484)

Closes #469

* Issue #469 is fixed

* one extra comment to code is added

* TemporaryFilePath context manager is added to facilitate tests

* flake8 complaints fixed

* Update docs of tempfilepath.py

* Fix flake8

Co-authored-by: Pieter Marsman <pietermarsman@gmail.com>
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.

2 participants