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

PR: Use a function to create and get our temporary directory #7829

Merged
merged 19 commits into from
Sep 10, 2018

Conversation

arielrossanigo
Copy link
Contributor

Pull Request Checklist

  • Read and followed this repo's Contributing Guidelines
  • Based your PR on the latest version of the correct branch (master or 3.x)
  • Followed PEP8 for code style
  • Ensured your pull request hasn't eliminated unrelated blank lines/spaces,
    modified the spyder/defaults directory, or added new icons/assets
  • Wrote at least one-line docstrings for any new functions
  • Added at least one unit test covering the changes, if at all possible
  • Described your changes and the motivation for them below
  • Noted what issue(s) this pull request resolves, creating one if needed

Description of Changes

The main change is that temp folder used by Spyder is not more removed when application closes, mainly because when multiple instances of Spyder are running and one of them is closed, temp folder were remove, and remaining ones didn't detect this action.

Another related change is that, every time temp folder is needed, a function to ensure that the folder exists was created. This was implemented using a function and removing a constant called TEMPDIR. This can cause that another projects or branches that have been using this constant crashes.

Because tempdir is not more removed when application closes, now some code to ensure that files are removed after been used was added. The points where this was done are:

  • When closing a client the stderr file is removed (if not related clients remains alive)
  • When is_module_installed finish

Issue(s) Resolved

Fixes #7800

@pep8speaks
Copy link

pep8speaks commented Sep 4, 2018

Hello @arielrossanigo! Thanks for updating the PR.

Line 56:72: W291 trailing whitespace

Line 427:64: W291 trailing whitespace
Line 428:78: W291 trailing whitespace

Line 117:80: E501 line too long (80 > 79 characters)
Line 122:80: E501 line too long (80 > 79 characters)

Comment last updated on September 10, 2018 at 17:05 Hours UTC

@CAM-Gerlach
Copy link
Member

Thanks for your contribution! I haven't examined the logic in detail, but while the Travis and CircleCI tests are apparently all failing before the tests run due to an unrelated issue (and so cannot be determined whether they have the same problem), the AppVeyor tests are indeed clearly failing due to this change.

Also, as a fairly substantial change, unless the issue is serious and widespread this should probably go to master, but that's ultimately up to @ccordoba12 so don't change anything yet.

@CAM-Gerlach CAM-Gerlach changed the title Fix issue 7800 PR: Fix issue 7800 Sep 5, 2018
@ccordoba12 ccordoba12 changed the title PR: Fix issue 7800 PR: Use a function to get/create our temporary directory Sep 5, 2018
@ccordoba12
Copy link
Member

Thanks a lot @arielrossanigo for your contribution!! I'll review it when I get back to Colombia (I'm waiting for my flight in Rio).

I'll take a look at our failing tests there too.

@arielrossanigo
Copy link
Contributor Author

I've take a look at logs and I've just found two main issues:

  • In Windows, when I tried to remove stderr file, it is blocked. I'm trying to figure it out how to fix that.
  • The error in Travis and Python 2.7 have to do with monkeypatch. I'm trying to reproduce the environment and fix this issue.

If you already deal with something like that let me know.

@arielrossanigo
Copy link
Contributor Author

The problem related to monkeypatch was solved.

Now I'm trying to figure it out how to solve the problem with file blocks and Windows... The main issue here is that I cannot find a place where can I close the file safely. I wanna try remove the files at all and replace them with StringIO but I'm not so sure what tests covers the use of stderr file.

@ccordoba12
Copy link
Member

@arielrossanigo, please rebase or merge with 3.x. The errors in our tests are fixed now.

@arielrossanigo
Copy link
Contributor Author

@ccordoba12 Finally I could fix the problems in Windows. Now, the only test failing has nothing to do with this PR.

@CAM-Gerlach
Copy link
Member

@arielrossanigo See #7856 , should get fixed soon.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Great job @arielrossanigo!! This is a really great improvement over what we had previously.

Most of my comments are purely cosmetic, with the only important change being the request to move get_stderr_file_handle to utils/misc.py.

spyder/plugins/ipythonconsole.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole.py Outdated Show resolved Hide resolved
spyder/plugins/tests/test_ipythonconsole.py Outdated Show resolved Hide resolved
spyder/plugins/tests/test_ipythonconsole.py Outdated Show resolved Hide resolved
spyder/plugins/tests/test_ipythonconsole.py Outdated Show resolved Hide resolved
spyder/plugins/tests/test_ipythonconsole.py Outdated Show resolved Hide resolved
spyder/plugins/tests/test_ipythonconsole.py Outdated Show resolved Hide resolved
spyder/utils/programs.py Outdated Show resolved Hide resolved
spyder/utils/programs.py Show resolved Hide resolved
@ccordoba12 ccordoba12 added this to the v3.3.2 milestone Sep 10, 2018
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks a lot @arielrossanigo! This is a really great contribution!

@ccordoba12 ccordoba12 changed the title PR: Use a function to get/create our temporary directory PR: Use a function to create and get our temporary directory Sep 10, 2018
@ccordoba12 ccordoba12 merged commit a3aa1a6 into spyder-ide:3.x Sep 10, 2018
ccordoba12 added a commit that referenced this pull request Sep 10, 2018
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.

4 participants