-
Notifications
You must be signed in to change notification settings - Fork 24
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
Use pyface.undo imports in apptools.undo instead of redefining the classes #272
Conversation
instead of simply raising a deprecation warning and redefining the modules/classes Note that the tests have been removed as this module is now simply a stub, exposing the functionality in pyface.undo, and will be removed in the future modified: apptools/undo/abstract_command.py modified: apptools/undo/action/abstract_command_stack_action.py modified: apptools/undo/action/api.py modified: apptools/undo/action/command_action.py modified: apptools/undo/action/redo_action.py modified: apptools/undo/action/undo_action.py modified: apptools/undo/api.py modified: apptools/undo/command_stack.py modified: apptools/undo/i_command.py modified: apptools/undo/i_command_stack.py modified: apptools/undo/i_undo_manager.py deleted: apptools/undo/tests/__init__.py deleted: apptools/undo/tests/test_command_stack.py deleted: apptools/undo/tests/testing_commands.py modified: apptools/undo/undo_manager.py
new file: docs/releases/upcoming/272.deprecation.rst
Ignore F401 errors anywhere in apptools.undo.* modified: apptools/undo/api.py modified: setup.cfg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
""" This is called by the command stack to undo the command. """ | ||
|
||
raise NotImplementedError | ||
from pyface.undo.api import AbstractCommand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For maintaining backward compatibility, I'd think the safest thing to do would be from pyface.undo.abstract_command import *
, assuming the module is moved to pyface as is. e.g. this breaks from apptools.undo.abstract_command import ICommand
. We believe no one does that, but it costs little to do the safer approach. 🤞 this does not break anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rahulporuri should we make this change before I push ahead with the release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd actually prefer breaking such imports now - which will make the eventual transition to pyface
trivial - and cleaner/better because those imports are antipatterns anyway.
fixes #271
This PR imports the relevant classes from
pyface.undo
andpyface.undo.action
instead of redefining the classes. Where possible, we import frompyface.undo.api
andpyface.undo.action.api
.Note that the tests have been removed as this module is now simply a stub, exposing the functionality in
pyface.undo
, and will be removed in the future.Checklist