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

Use pyface undo instead of apptools.undo #507

Merged
merged 10 commits into from
Dec 17, 2020
Merged

Conversation

aaronayres35
Copy link
Contributor

@aaronayres35 aaronayres35 commented Dec 16, 2020

fixes #503

As of Pyface 7.2.0, the undo/redo functionality that used to be specific to apptools is now available through pyface. With the upcoming release of apptools, it will be deprecated.

This PR simply replaces all uses of apptools.undo with pyface.undo and updates file names / documentation accordingly.

Note that after this PR, the only use of apptools in the code base will be here:

try:
# try to extract file info from mimedata
# XXX this is for compatibility with what wx does
from apptools.io.api import File
obj = [File(path=path) for path in files]
except ImportError:
warnings.warn("apptools is unavailable", ImportWarning)

Maybe this code could use pathlib instead as mentioned enthought/apptools#143?

apptools is still listed as a dependency in ci/edmtool.py but maybe that can be removed?

Also, note: this PR will cause merge conflicts with #494, but they should be simple enough to resolve (just replace apptools with pyface in the example code that mentions it on 494)

@codecov-io
Copy link

codecov-io commented Dec 16, 2020

Codecov Report

Merging #507 (9a96c32) into master (f83869e) will increase coverage by 0.00%.
The diff coverage is 40.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #507   +/-   ##
=======================================
  Coverage   30.62%   30.63%           
=======================================
  Files         206      208    +2     
  Lines       17830    17832    +2     
  Branches     2454     2454           
=======================================
+ Hits         5461     5463    +2     
  Misses      12036    12036           
  Partials      333      333           
Impacted Files Coverage Δ
enable/tools/apptools/api.py 0.00% <0.00%> (ø)
enable/tools/pyface/api.py 0.00% <0.00%> (ø)
enable/tools/pyface/move_command_tool.py 86.95% <ø> (ø)
enable/tools/pyface/resize_command_tool.py 85.18% <ø> (ø)
enable/tools/pyface/undo_tool.py 95.23% <ø> (ø)
enable/tools/apptools/__init__.py 100.00% <100.00%> (ø)
enable/tools/pyface/command_tool.py 84.61% <100.00%> (ø)
enable/tools/pyface/commands.py 97.82% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f83869e...9a96c32. Read the comment docs.

@jwiggins
Copy link
Member

Concerning the drag and drop handling for Qt... I think some spelunking will be required to see how hard it would be to get rid of apptools (who's using it, what are they doing with the File objects, etc.). Although we're prepping for a major release, so now is probably the best time to "break" it.

Copy link
Member

@jwiggins jwiggins left a comment

Choose a reason for hiding this comment

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

A question and a comment. This should be good to go once the question is resolved.

enable/tools/pyface/api.py Show resolved Hide resolved
examples/enable/tools/button_tool.py Outdated Show resolved Hide resolved
@jwiggins
Copy link
Member

There was a slight misunderstanding. You should delete everything in enable.tools.apptools except __init__.py and api.py. api.py should forward imports from the new enable.tools.pyface.api module [as well as generating a warning].

@aaronayres35
Copy link
Contributor Author

There was a slight misunderstanding. You should delete everything in enable.tools.apptools except __init__.py and api.py. api.py should forward imports from the new enable.tools.pyface.api module [as well as generating a warning].

so the warning should be generated from the api or from the __init__.py? when deprecating apptools.undo itself I raised the warning in __init__.py, I'm not exactly sure what is the more correct thing to do

@jwiggins
Copy link
Member

__init__.py is probably the better place

Copy link
Member

@jwiggins jwiggins left a comment

Choose a reason for hiding this comment

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

One comment, otherwise LGTM

enable/tools/apptools/__init__.py Show resolved Hide resolved
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.

DeprecationWarning from importing apptools.undo
4 participants