Skip to content

catch OSError instead of FileNotFoundError in _get_executable_info to resolve #15399 #15413

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

Conversation

immaxchen
Copy link
Contributor

PR Summary

While finding available movie writers, if the executable is presented but
cannot be run, the import process will fail upon calling '_get_executable_info'
since it only handled CalledProcessError and FileNotFoundError.

In this commit, we catch OSError instead of FileNotFoundError in '__init__.py'
and log the error message in 'animation.py' to improve the import robustness.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

While finding available movie writers, if the executable is presented but
cannot be run, the import process will fail upon calling '_get_executable_info'
since it only handled CalledProcessError and FileNotFoundError.

In this commit, we catch OSError instead of FileNotFoundError in '__init__.py'
and log the error message in 'animation.py' to improve the import robustness.
@ImportanceOfBeingErnest
Copy link
Member

I think we should still catch FileNotFoundError as well.

@immaxchen
Copy link
Contributor Author

Should we? I thought OSError covers FileNotFoundError already.

@ImportanceOfBeingErnest
Copy link
Member

Oh yes, sorry; that's the built-in FileNotFoundError so catching OSError should be fine.

@tacaswell tacaswell added this to the v3.2.0 milestone Oct 13, 2019
@tacaswell tacaswell merged commit 19d589c into matplotlib:master Oct 13, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 13, 2019
@tacaswell
Copy link
Member

Thanks @immaxchen ! Congratulations on your first merged Matplotlib pull request 🎉

Hopefully we will hear from you again!

tacaswell added a commit that referenced this pull request Oct 13, 2019
…413-on-v3.2.x

Backport PR #15413 on branch v3.2.x (catch OSError instead of FileNotFoundError in _get_executable_info to resolve #15399)
@immaxchen
Copy link
Contributor Author

Thanks to Ernest's guidance on the solution, I'll try to keep it up.
This is also the first PR in my life, thank you pals! 😄

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.

3 participants