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

Add deprecation warning for apptools.undo #250

Merged
merged 6 commits into from
Dec 10, 2020
Merged

Conversation

aaronayres35
Copy link
Contributor

fixes #243

Pyface 7.2 is about to be release and as a result apptools.undo needs to be deprecated. This PR adds a deprecation warning to apptools.undo.__init__.py and apptools.undo.action.__init__.py so that importing from apptools.undo will raise a deprecation warning. It also adds a test to verify deprecation warnings are raised when importing the api modules.

Checklist

  • Add a news fragment if this PR is news-worthy for end users. (see docs/releases/README.rst)

Copy link
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two somewhat orthogonal issues:
(1) apptools test suite is emitting this deprecation warning. I believe they all come from the undo's own test suite. With that, they are false alarms and should be handled either by filtering the warning or by modifying the imports such that the warning is not emitted.
(2) The tests added are failing precisely because apptools test suite has already caused the warning to be emitted at import time. Importing the test module itself requiring importing __init__ prior to the test being run. Trying to import apptools.undo.api is not going to execute the module again because imported modules are cached.

I am not sure it is worth the effort in testing these import-time warnings. It is important though to make sure the __init__'s can still run, i.e. there are no silly syntax errors and the like that would prevent the entire package from being imported. Since there are tests in apptools.undo, the apptools.undo.__init__ is covered. But I am not sure if apptools.undo.action.__init__ has been covered by tests, could you check please?

apptools/undo/__init__.py Show resolved Hide resolved
@aaronayres35
Copy link
Contributor Author

But I am not sure if apptools.undo.action.__init__ has been covered by tests, could you check please?

I ran coverage on the undo tests, without the test_deprecated tests I added and found that apptools.undo.action.__init__ is currently covered

@kitchoi
Copy link
Contributor

kitchoi commented Dec 10, 2020

I ran coverage on the undo tests, without the test_deprecated tests I added and found that apptools.undo.action.init is currently covered

Right, it is probably covered by unittest discovery itself, fair enough.
Shall we omit the tests on the deprecation warnings (IMO not worth the battle)? We should still filter in apptools's own test suite though.

@aaronayres35
Copy link
Contributor Author

Right, it is probably covered by unittest discovery itself, fair enough.
Shall we omit the tests on the deprecation warnings (IMO not worth the battle)? We should still filter in apptools's own test suite though.

I tried adding a filter I am still seeing a warning. The warning it says is coming from python3.6/unittest/loader.py:369

@kitchoi
Copy link
Contributor

kitchoi commented Dec 10, 2020

We should still filter in apptools's own test suite though.

Actually, we are fighting ourselves here. When we run the test suite, we set PYTHONWARNINGS to default, which will cause these deprecation warnings to be emitted when we discover the tests, exactly what we want when we want to detect deprecation warnings from other packages (e.g. traits).

That's all because the warning is in __init__, which unittest has to visit for test discovery. We might have to live with that noise. :/

@aaronayres35
Copy link
Contributor Author

That's all because the warning is in __init__, which unittest has to visit for test discovery. We might have to live with that noise. :/

Ah I see, that makes sense. I guess I will remove the warning filters then, as they aren't really doing anything

@kitchoi
Copy link
Contributor

kitchoi commented Dec 10, 2020

Yep, SGTM.

etstool.py Show resolved Hide resolved
@aaronayres35 aaronayres35 merged commit 0d3519d into master Dec 10, 2020
@aaronayres35 aaronayres35 deleted the deprecate-undo branch December 10, 2020 15:51
@rahulporuri
Copy link
Contributor

Should've looked at this before it was merged but this PR definitely needed a news fragment

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.

Deprecate apptools.undo after pyface 7.2.0 minor release
3 participants