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

Fix write_videofile error handling and update write_audiofile to be comparable #1164

Merged

Conversation

tburrows13
Copy link
Collaborator

@tburrows13 tburrows13 commented Apr 28, 2020

I found that #890 actually broke the catching of errors under the default settings (ie without a log file). This is because when there is no default settings, self.logfile is equal to sp.PIPE, which is just an integer, so it has no name attribute. With this PR, we make sure that we only deal with self.logfile if proc.communicate() returns None (if the return is simply empty, it returns b"", so that is accounted for).

I've included a test for the default case to ensure that this doesn't happen again.

Also, instead of getting the name of the log file and reopening it, I thought that it made more sense to use the existing file handler that is already open and simply seek back to the start and read it again.

I slightly updated the string handling as well, making it convert to bytes using .decode(), and one of the string matchers seemed to be wrong or least outdated (I checked what the actual error ffmpeg returns is), and with my change it matches successfully when bitrate="1").

Finally, I've applied #890 and all the above changes to ffmpeg_audiowriter as well, so now they should both be directly comparable.

  • I have provided code that clearly demonstrates the bug and that only works correctly when applying this fix
  • I have added suitable tests demonstrating a fixed bug or new/changed feature to the test suite in tests/
  • I have properly documented new or changed features in the documentation or in the docstrings
  • I have properly explained unusual or unexpected code in the comments around it
  • I have formatted my code using black -t py36

Because this is a small change, I'll merge it myself in a few days, unless anyone else says otherwise.

@tburrows13 tburrows13 added the bug-fix For PRs and issues solving bugs. label Apr 28, 2020
@coveralls
Copy link

coveralls commented Apr 28, 2020

Coverage Status

Coverage decreased (-0.05%) to 64.772% when pulling 46a7276 on tburrows13:fix-write_videofile-error-handling into 850a40e on Zulko:master.

@tburrows13 tburrows13 changed the title Fix write_videofile error handling Fix write_videofile error handling and update write_audiofile handling to be comparable Apr 29, 2020
@tburrows13 tburrows13 changed the title Fix write_videofile error handling and update write_audiofile handling to be comparable Fix write_videofile error handling and update write_audiofile to be comparable Apr 29, 2020
@tburrows13 tburrows13 merged commit 843c0ae into Zulko:master May 7, 2020
@tburrows13 tburrows13 deleted the fix-write_videofile-error-handling branch May 7, 2020 20:08
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants