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

Updated deprecated test approach + minor changes #89

Merged
merged 11 commits into from
Apr 10, 2023
Merged

Updated deprecated test approach + minor changes #89

merged 11 commits into from
Apr 10, 2023

Conversation

lbreede
Copy link
Contributor

@lbreede lbreede commented Apr 6, 2023

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
Copy link
Contributor Author

lbreede commented Apr 6, 2023

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

lbreede commented Apr 7, 2023

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
Copy link
Contributor Author

lbreede commented Apr 7, 2023

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
Copy link
Member

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

@almarklein almarklein merged commit 2f3403a into imageio:master Apr 10, 2023
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