search

koodaamo / tnefparse

a TNEF decoding library written in python, without external dependencies
GNU Lesser General Public License v3.0
49 stars 37 forks source link

deprecation warning #98

Closed jugmac00 closed 3 years ago

jugmac00 commented 3 years ago

e.g. run tox - e py38

tests/test_decoding.py::test_zip
  /home/jugmac00/Projects/tnef-parallel/tnefparse/tnefparse/tnef.py:403: DeprecationWarning: passing bytes to tnef.to_zip will be deprecated, pass a TNEF object instead
    warnings.warn(msg, DeprecationWarning)
1nF0rmed commented 3 years ago

Hey! Do you want to update the tests to use a TNEF object instead of passing bytes?

jugmac00 commented 3 years ago

Sounds like a good plan!

1nF0rmed commented 3 years ago

If no one is assigned this issue then I would like to work on this

jugmac00 commented 3 years ago

Sure, go ahead!

jugmac00 commented 3 years ago

Do not forget to add yourself to HISTORY.rst.

1nF0rmed commented 3 years ago

I've made a PR with the fix. Let me know if you have any questions

jugmac00 commented 3 years ago

Hi @1nF0rmed,

I am very sorry - I made a mistake.

I had another look at the code, and we have to keep the deprecated way, otherwise we loose test coverage.

In other words: We have deprecated passing in bytes, but for some more time, we have to support it, and so we have to test this behavior.

Currently, every test run shows a deprecation warning, which is not what I'd like to see.

But luckily, pytest offers a solution to this problem: We can assert that a deprecation warning is issued within the test and get rid of it after the test run at once! https://docs.pytest.org/en/stable/warnings.html

Can you please update the test code to something like the following code?

def test_zip():
    # remove this test once tnef.to_zip(bytes) is no longer supported
    with pytest.deprecated_call():
        with open(datadir + os.sep + 'one-file.tnef', "rb") as tfile:
            zip_data = to_zip(tfile.read())
            with tempfile.TemporaryFile() as out:
                out.write(zip_data)

Thank you and sorry for the confusion.

P.S.: Formatting is broken... I think GitHub has some issues - also commenting is broken for me - I only could answer to this issue by email.

1nF0rmed commented 3 years ago

Hey, I was confused as well when I saw the test coverage drop. Will be updating this to use the warning capture instead.

1nF0rmed commented 3 years ago

Hey @jugmac00, I've updated the pull request #99
Let me know if you have any questions.

jugmac00 commented 3 years ago

Thank you!

fixed by #99