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

move example helper modules into examples #805

Merged
merged 6 commits into from
Jun 21, 2021

Conversation

aaronayres35
Copy link
Contributor

@aaronayres35 aaronayres35 commented Apr 28, 2021

closes #793
This PR simply moves the enable.example_application, enable.example_canvas, and enable.example_support modules to sit in the enable/examples directory. I then did a VSCode find and replace for enable.example_support with enable.examples.example_support, etc.

Note, as mentioned in this comment: #793 (comment) this may be a breaking change elsewhere as it appears we have down stream users who had directly been importing from these modules. As a result we may not want to push ahead with these changes...

Perhaps the old modules should be left around, but simply import from the new modules in enable/examples and raise some sort of warning? A deprecation warning doesn't really feel correct as it seems to me its more of a "you shouldn't be importing from here" kind of warning.

EDIT: I have added stub modules at the old file locations which simply import objects from the new modules and raise a deprecation warning. In the deprecation warning, I did not mention these modules were moved, as we don't want users importing them from anywhere! So I just said importing from here is deprecated and the module will be removed in a future release. Perhaps I should say explicitly don't do this?

@rahulporuri rahulporuri self-requested a review May 3, 2021 05:20
@rahulporuri rahulporuri changed the title move example helper moodules into examples move example helper modules into examples May 11, 2021
Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

Almost LGTM - with a bunch of comments/suggestions

Comment on lines 14 to 16
This module provides a simple Pyface application that can be used by examples
in places where a DemoFrame is insufficient.
in places where a DemoFrame is insufficient. Note this has been moved to
sit in enable/examples. This module is kept for backwards compatibility.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go with the following for the module docstring - and the warning message as well.

Importing from this module is deprecated. Please use `enable.examples.example_application`
instead. This backwards compatibility stub will be removed in Enable 6.0.

We need an issue to track the removal of these stubs in Enable 6.0

Copy link
Contributor

Choose a reason for hiding this comment

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

let's use the above template for the module docstring and warning message in the other stub modules as well.

Copy link
Contributor Author

@aaronayres35 aaronayres35 Jun 21, 2021

Choose a reason for hiding this comment

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

Should we say "Please use enable.examples.example_application instead." if we are making the modules private? Theoretically the suggested change is "stop doing that" as opposed to "update your imports"

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh. You're right. Let's update it to

This module is meant for internal use only and it is not meant for use in library code.
Importing from this module is deprecated and it will be removed in Enable 6.0.
We highly recommend that you update your code and vendorize what is necessary.

Comment on lines 21 to 35
# FIXME - it should be enough to do the following import, but because of the
# PyQt/traits problem (see below) we can't because it would drag in traits too
# early. Until it is fixed we just assume wx if we can import it.
# Force the selection of a valid toolkit.
# import enable.toolkit
if not ETSConfig.toolkit:
for toolkit, toolkit_module in (("wx", "wx"), ("qt4", "PyQt4")):
try:
exec("import " + toolkit_module)
ETSConfig.toolkit = toolkit
break
except ImportError:
pass
else:
raise RuntimeError("Can't load wx or qt4 backend for Chaco.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this isn't necessary now.

@aaronayres35 aaronayres35 requested a review from rahulporuri June 21, 2021 14:12
Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM. I think making them private is the right thing to do. Let's see if anyone complains.

@aaronayres35 aaronayres35 merged commit f8cfc05 into master Jun 21, 2021
@aaronayres35 aaronayres35 deleted the move-example-helper-modules branch June 21, 2021 20:43
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.

Consider moving "enable.example_*" modules into "enable.examples" submodule
2 participants