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

Fixed problems with proc package #1016

Closed
wants to merge 2 commits into from
Closed

Conversation

Kaszanas
Copy link

@Kaszanas Kaszanas commented Sep 30, 2019

Unit testing yielded couple of errors during the processing of the video.
All of them were connected to the use of proc package.

  • If this is a bugfix, I have provided code that clearly demonstrates the problem and that works when used with this PR
  • I have added a test to the test suite, if necessary
  • I have properly documented new or changed features in the documention, or the docstrings
  • I have properly documented unusual changes to the code in the comments around it
  • I have made note of any breaking/backwards incompatible changes

Unit testing yielded couple of errors during the processing of the video.
All of them were connected to the use of proc package.
@tburrows13 tburrows13 marked this pull request as ready for review October 1, 2019 13:37
@tburrows13
Copy link
Collaborator

Sorry, I didn't mean to get rid of the 'draft' status.

@Kaszanas
Copy link
Author

Kaszanas commented Oct 1, 2019

@tburrows13 not a problem I haven't marked it as ready for review because I already fixed the problem and I do not have the exact code that is required to get the same result when it comes to reproducing the errors that were involved.

I am not sure when I will be able to recreate those. If the changes are acceptable "as is" then I think that it is reasonable to merge.

Otherwise if any more code is needed to prove that it is a fix. It will have to wait.

There were exactly 3 errors while processing video files.
I will try to post the resulting errors here ASAP.

@Julian-O
Copy link
Contributor

Julian-O commented Oct 3, 2019

I am confused by the purpose of the check before terminating:

if self.proc.poll() is not None:
    self.proc.terminate()

Firstly, is there any harm in skipping the check and just calling terminate every time?

Secondly, the return code definition says:

 A None value indicates that the process hasn’t terminated yet.

So, this seems to call terminate only if it has already been terminated.

I am not an expert at subprocess, so this could just be my confusion rather than a bug.

@tburrows13 tburrows13 mentioned this pull request Apr 24, 2020
7 tasks
@tburrows13 tburrows13 added the bug-fix For PRs and issues solving bugs. label May 7, 2020
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 67.317% when pulling d8e1aea on Kaszanas:master into b2ed1f6 on Zulko:master.

@tburrows13
Copy link
Collaborator

Adding .wait() was added as part of another PR some time ago so is no longer shown here in the 'Files changed'. Checking .poll() has come up again in #1296.

@mondeja
Copy link
Collaborator

mondeja commented Jan 14, 2021

Since @Kaszanas has not posted the errors, #1296 has been merged and the source code has changed a lot since this pull, I'm closing this. Feel free to reopen it if your errors persist.

@mondeja mondeja closed this Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix For PRs and issues solving bugs.
Development

Successfully merging this pull request may close these issues.

None yet

5 participants