-
Notifications
You must be signed in to change notification settings - Fork 8
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 if condition to check for embedded resources in pdf and raise error if file not found #18
Conversation
* Moved checking for missing file in __new__ * added test to see if class is catching error of missing file
This looks very complicated. Why is test code in the main function? I'm not sure this is the best solution. |
I don't know if you will approve this, but for testing |
This doesn't look right to me. You can look at Alexis' original repo on how he did the checking. |
The checking part was easy and straight forward see here, but I had to do this to enable unittest to verify that class FacturX is raising an Error. |
You can't change the main code just to enable tests. I'm sure there is another way. |
I did a lot of research and couldn't find any other. The reason for the change is, we cannot assert (or do anything) after an error is raised. And since we will be enabling CI in future, we need either the test to pass or remove this check(normal check without changing main code). |
You can look at this SO question to get an idea on how to test for correct Exceptions. |
Obviously I did and implemented it, but the solution is wrong. It is saying something completely different and doesn't apply here. |
Check this https://stackoverflow.com/a/31918982 |
Maybe choose another issue to work on. I'll tackle this one later. Thanks anyways. |
What's wrong with this solution that seems to work well enough?
|
Now it seems to work, strange. My bad. But this doesn't
|
Yeah so we need to make this change to raise the |
Figured it out
After merge we can close #16