Deprecate usage of get_app functions to get QApplication or NapariApplication instances
#7269
Conversation
…riApplication' instances Use 'get_qapp' for 'QApplication' and 'get_app_model' for 'NapariApplication'
get_app functions to get QApplication or NapariApplication instances
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7269 +/- ##
=======================================
Coverage ? 92.74%
=======================================
Files ? 623
Lines ? 57134
Branches ? 0
=======================================
Hits ? 52991
Misses ? 4143
Partials ? 0 ☔ View full report in Codecov by Sentry. |
| qapp = get_qapp() | ||
| with pytest.warns(FutureWarning): | ||
| deprecated_qapp = get_app() | ||
| assert qapp == deprecated_qapp | ||
|
|
||
|
|
||
| @pytest.mark.skipif(os.name != 'Windows', reason='Windows specific') |
There was a problem hiding this comment.
Just in case, while checking the test addition over this file, I noticed this skip, shouldn't it be something like:
@pytest.mark.skipif(os.name != 'nt', reason='Windows specific')?
As it is, the test is skipped on Windows. However, if you do the skip definition change above the test is run but it fails on Windows. Not related to the changes here but maybe worthy of an issue 🤔?
There was a problem hiding this comment.
Created #7278 to follow up this 👍
napari/_app_model/_tests/test_app.py
Outdated
| assert app.name == 'test_app' | ||
| assert list(app.menus) | ||
| assert list(app.commands) | ||
| # assert list(app.keybindings) # don't have any yet | ||
| with pytest.warns(FutureWarning): |
There was a problem hiding this comment.
Please use match argument with matching string to check if a proper future warning is emmited
napari/_qt/_tests/test_app.py
Outdated
| with pytest.warns(FutureWarning): | ||
| deprecated_qapp = get_app() |
napari/utils/_testsupport.py
Outdated
| @@ -143,7 +143,7 @@ def pytest_runtest_makereport(item, call): | |||
| def mock_app(): | |||
There was a problem hiding this comment.
I wonder if this should be changed to mock_app_model now?
|
|
||
| from app_model import Application | ||
|
|
||
| from napari._app_model.actions._layerlist_context_actions import ( | ||
| LAYERLIST_CONTEXT_ACTIONS, | ||
| LAYERLIST_CONTEXT_SUBMENUS, | ||
| ) | ||
| from napari.utils.translations import trans | ||
|
|
||
| APP_NAME = 'napari' | ||
|
|
There was a problem hiding this comment.
Lets change the NapariApplication method get_app to get_app_model, and update the mock_app as well. Thanks!
napari/_qt/qt_event_loop.py
Outdated
| @@ -61,7 +61,59 @@ def set_app_id(app_id): | |||
| _IPYTHON_WAS_HERE_FIRST = 'IPython' in sys.modules | |||
|
|
|||
|
|
|||
| def get_app( | |||
| def get_app(*args, **kwargs) -> QApplication: | |||
| """Get or create the Qt QApplication. | |||
There was a problem hiding this comment.
I feel like we can just delete this docstring and just say, deprecated, use get_qapp.
Maybe we could add a todo to say delete in napari 0.6.0 (I assume next major release dep is fine?) Also lets add the todo to the other get_app as well!
napari/_app_model/_app.py
Outdated
| @@ -57,5 +59,19 @@ def _public_types(module): | |||
|
|
|||
|
|
|||
| def get_app() -> NapariApplication: | |||
| """Get the Napari Application singleton.""" | |||
There was a problem hiding this comment.
As with the other, lets just say deprecated, use get_app_model, to deprecate 0.6.0
… 'NapariApplication.get_app_model' and update deprecated methods docstrings
Created a couple of PRs for that 👍:
One pending repo to update from my quick search over the napari org is https://github.com/napari/napari-language-packs/tree/main Seems like over there the translations strings are kept? Maybe an update command needs to be run so the base strings get updated/added to the Is there any other place where changes need to be done? Let me know! |
|
We should decide if we want to remove get_app in |
|
I had a look at this and everything looks good. I agree we should add the version it'll be removed in, and I think it should be |
napari/_qt/qt_event_loop.py
Outdated
| def get_app( | ||
| def get_app(*args, **kwargs) -> QApplication: | ||
| """Get or create the Qt QApplication. Now deprecated, use `get_qapp`.""" | ||
| # TODO: Remove in napari 0.6.0 |
There was a problem hiding this comment.
Sorry nit, can you put this comment above the get_app function?
Co-authored-by: Lucy Liu <jliu176@gmail.com>
…comment Co-authored-by: Lucy Liu <jliu176@gmail.com> Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com> Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
No idea about this one, maybe @goanpeca knows? 🙏 |
…d of `mock_app_model` and `get_app_model` (#7283) # References and relevant issues CI over main is failing due to the issue this PR fixes # Description Since PR #7269 and PR #7266 where being worked on in parallel merging both caused some tests to fail (due to their usage of `mock_app` and `get_app`)
# References and relevant issues Part of napari/napari#6054 # Description Use `get_qapp` instead of `get_app` (being deprecated over PR napari/napari#7269) --------- Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
References and relevant issues
Closes #6054
Description
Use
get_qappto get aQApplicationinstance andget_app_modelfor aNapariApplicationone. Add a deprecation warning when trying to access via the respectiveget_appfunctions without removing them.Using the
get_appfunctions with the changes here should give you a warning: