-
Notifications
You must be signed in to change notification settings - Fork 170
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
Added proper error handling when trying to return false templates #765
Conversation
Such a simple fix that I didn't have time to research yet. Thanks! (The docker build check fail is a thing that's happened due to something happening with the container and not your code itself.) Would you mind adding in some unit tests to help spot future issues if this happens again? |
Of course, One second! |
I didn't edit my last comment fast enough! No, it's a result of something to do with how the Docker container processes the PIP installs, so it's not your fault. I'll merge regardless of that check (and why it's optional right now.) |
Alright just pushed test coverage! |
Thanks! One second tiny ask (sorry!)... You test for failure, can you test for success? |
If you would like more test cases on this one I can do that for you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing this and making the tweaks. I'll merge it in.
Have a great rest of the week!
Oh and for being a first-time contributor too! Congrats! |
Thank you, you have a good one as well! |
What GitHub issue does this PR apply to?
Resolves #764
What changed and why?
Added a try catch block so the end user doesn't get that 500 internal server error, this is good for the future too if a user is trying to access an invalid API url.
Checklist
Any additional comments or things to be aware of while reviewing?
Happy to help!