imageio / imageio-ffmpeg

FFMPEG wrapper for Python
BSD 2-Clause "Simplified" License
221 stars 50 forks source link

Updated deprecated test approach + minor changes #89

Closed lbreede closed 1 year ago

lbreede commented 1 year ago

Major changes

  1. Replaced with warns(None) as warnings: with a new syntax by importing the warnings module and with warnings.catch_warnings(record=True) as warnings_:. This turned one of the pytest warning to a pytest passed.

Minor changes

  1. Fixed type from FFMEG to FFMPEG
  2. Replaced the variable i with _ for for-loops that do not utilize i
  3. Replaced variable names with more descriptive ones: w -> width, h -> height, p -> process

Patch changes

  1. Ran isort .
  2. Ran black .
lbreede commented 1 year ago

This is my first PR on a public repo and I couldn't find contribution guidelines. I'm happy to revert any changes that do not comply with any guidelines I might be unaware of, especially since I mindlessly ran isort and black over everything.

almarklein commented 1 year ago

Hey @lbreede 👋 Thanks for this PR!

I added a section for developers to the readme. That was indeed missing.

In general its more convenient to create separate smaller PR's for unrelated changes. That way the "easy" changes can be merged quickly, so changes that need more work or discussion won't hold up the rest.

In this particular case I agree with all changes. I don't care much about import sorting, but I don't think it hurts either 😄

almarklein commented 1 year ago

There are a few linting errors (you can check locally with invoke lint). The other CI jobs seem to timeout on downloading ffmpeg, not sure why this is 🤔

lbreede commented 1 year ago

Hi @almarklein, thanks for the warm welcome! I've addressed the linting error and will look into the CI issues. I guess here's another good reason for smaller PRs, I would know where to look 🤦 I'll see if I can get those running again otherwise as you said 😄

lbreede commented 1 year ago

Thanks for adding the developers section, super helpful and I had actually never used invoke. All tests are passing on my end now. Let me know if there's anything else you want me to add/remove.

almarklein commented 1 year ago

Thanks! Looking good! (The pypy build seems to have been broken before this PR.)