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

Added if condition to check for embedded resources in pdf and raise error if file not found #18

Closed
wants to merge 3 commits into from

Conversation

duskybomb
Copy link

Figured it out
After merge we can close #16

@duskybomb duskybomb changed the title Added if condition to check for embedded resources in pdf Added if condition to check for embedded resources in pdf and raise error if file not found May 22, 2018
 * Moved checking for missing file in __new__
 * added test to see if class is catching error of missing file
@m3nu
Copy link
Collaborator

m3nu commented May 23, 2018

This looks very complicated. Why is test code in the main function? I'm not sure this is the best solution.

@duskybomb
Copy link
Author

duskybomb commented May 23, 2018

I don't know if you will approve this, but for testing file_doesn't_exist case I had to move checking to __new__ so that I can return something and test doesn't fail because of Error we are raising.

@m3nu
Copy link
Collaborator

m3nu commented May 23, 2018

This doesn't look right to me. You can look at Alexis' original repo on how he did the checking.

@duskybomb
Copy link
Author

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.

@m3nu
Copy link
Collaborator

m3nu commented May 23, 2018

You can't change the main code just to enable tests. I'm sure there is another way.

@duskybomb
Copy link
Author

duskybomb commented May 23, 2018

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).

@m3nu
Copy link
Collaborator

m3nu commented May 23, 2018

You can look at this SO question to get an idea on how to test for correct Exceptions.

@duskybomb
Copy link
Author

Obviously I did and implemented it, but the solution is wrong. It is saying something completely different and doesn't apply here.

@duskybomb
Copy link
Author

Check this https://stackoverflow.com/a/31918982

@m3nu
Copy link
Collaborator

m3nu commented May 23, 2018

Maybe choose another issue to work on. I'll tackle this one later. Thanks anyways.

@m3nu m3nu closed this May 23, 2018
@m3nu
Copy link
Collaborator

m3nu commented May 23, 2018

What's wrong with this solution that seems to work well enough?

    def test_input_error(self):
        with self.assertRaises(TypeError):
            FacturX('non-existant.pdf')

@duskybomb
Copy link
Author

Now it seems to work, strange. My bad.

But this doesn't

import mymod

class MyTestCase(unittest.TestCase):
    def test1(self):
        self.assertRaises(SomeCoolException, mymod.myfunc)

@duskybomb
Copy link
Author

duskybomb commented May 23, 2018

Yeah so we need to make this change to raise the FileNotFoundError this commit and hence tweak the test a little

@duskybomb duskybomb deleted the issue/16 branch May 29, 2018 04:32
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.

Error with PyPDF2
2 participants