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 new chaco.plots submodule #738

Merged
merged 19 commits into from
Jun 2, 2021
Merged

Add new chaco.plots submodule #738

merged 19 commits into from
Jun 2, 2021

Conversation

aaronayres35
Copy link
Contributor

This PR would close #700

It is meant to demonstrate the plan of action for part of the proposed re-org.
If we decide to go ahead with this re-org this draft PR can be marked ready for review / fixed up and merged.

Comment on lines 14 to 19
warnings.warn(
"This module has been moved to sit in chaco/plots and this stub module has"
" been kept for backwards compatibility. Importing from this module is"
" deprecated, please import needed objects from chaco.api instead",
DeprecationWarning
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: "Import-time deprecation warnings risk being hidden from test runs."
We may want to update how we are raising these deprecation warnings...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should also update the warning message to be explicit that these modules will be removed in chaco V6 or V7 once that is decided on

@@ -291,7 +291,7 @@
from .color_mapper import ColorMapper, ColorMapTemplate
from .discrete_color_mapper import DiscreteColorMapper
from .transform_color_mapper import TransformColorMapper
from .horizon_plot import BandedMapper
from .plots.horizon_plot import BandedMapper
Copy link
Contributor

Choose a reason for hiding this comment

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

it might look cleaner in the final version to create a chaco.plots.api and import all of these plot types from there in a single location in the chaco.api module - instead of being spread around like they are at the moment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the latest commit. The only things I am unsure about are ColorBar, render_markers, and also BandedMapper from chaco.plots.horizon_plot. In other words which section should they go in for chaco.api, should chaco.plots.api be divided into sections to separate them out? Should they be exposed in the chaco.plots.api at all, or should chaco.plots.api be kept just for -___Plot___ classes?

This was referenced May 28, 2021
@aaronayres35
Copy link
Contributor Author

I am going to convert this PR from a draft PR to a real PR as we do aim to merge it eventually now

@aaronayres35 aaronayres35 marked this pull request as ready for review June 1, 2021 22:18
chaco/barplot.py Outdated Show resolved Hide resolved
chaco/barplot.py Outdated Show resolved Hide resolved
chaco/horizon_plot.py Outdated Show resolved Hide resolved
chaco/plot.py Outdated Show resolved Hide resolved
chaco/scatterplot.py Outdated Show resolved Hide resolved
chaco/plots/api.py Outdated Show resolved Hide resolved
chaco/plots/api.py Outdated Show resolved Hide resolved
chaco/plots/api.py Outdated Show resolved Hide resolved
chaco/api.py Show resolved Hide resolved
chaco/api.py Show resolved Hide resolved
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 with a couple of minor comments.

@@ -121,6 +121,29 @@

doc_ignore = {
"*/tests",
"chaco/barplot.py",
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to add a reason here as to why these are ignored. I think we can create an issue - regarding the removal of the stubs - and link that here.

chaco/plot.py Outdated Show resolved Hide resolved
@aaronayres35 aaronayres35 merged commit 600d8e9 into master Jun 2, 2021
@aaronayres35 aaronayres35 deleted the add-plots-submodule branch June 2, 2021 15:35
aaronayres35 added a commit that referenced this pull request Jun 2, 2021
* add new chaco.plots submodule

* move contour_line_plot.py and contour_poly_plot.py into chaco/plots/contour

* import from api in tests

* move relevant tests into chaco/plots/tests

* add stub files at old file locations which raise Deprecation warning and import objects from their new chaco/plots modules

* flake8

* update warning message

* set stacklevel=2 in deprecation warnings

* add a chaco.plots.api

* typo

* update deprecation warning messages, and remove module docstrings

* Apply suggestions from code review

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>

* ignore stub modules when generating api docs

* flake8

* update api module docstrings

* reorder imports

* suggestions from code review

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>
aaronayres35 added a commit that referenced this pull request Jun 2, 2021
* FIX: fix scrollbar demo and update docstring (#489)

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>

* Add new chaco.plots submodule (#738)

* add new chaco.plots submodule

* move contour_line_plot.py and contour_poly_plot.py into chaco/plots/contour

* import from api in tests

* move relevant tests into chaco/plots/tests

* add stub files at old file locations which raise Deprecation warning and import objects from their new chaco/plots modules

* flake8

* update warning message

* set stacklevel=2 in deprecation warnings

* add a chaco.plots.api

* typo

* update deprecation warning messages, and remove module docstrings

* Apply suggestions from code review

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>

* ignore stub modules when generating api docs

* flake8

* update api module docstrings

* reorder imports

* suggestions from code review

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>

* add pyproject.toml to MANIFEST.in (#747)

* Consolidate overlays into "chaco.overlays" (#741)

* copy modules into chaco/overlays

* make original modules stubs raising deprecation warnings

* copy layers into overlays

* fully populate chaco.overlays.api and chaco.api

* flake8

* update tox.ini by running flake8 -q

* somehow this is also needed in tox.ini but I did not see this flake8 error locally

* remove bad trailing comma

* move data label test to sit in chaco/overlays/tests

* add stub modules in chaco/layers importing from new location and adding deprecation warnigns.  Also expose layers in the chaco.overlays.api

* add layers to the chaco.api as well

* old files that were moved have since been changed on master and did not get updated in merge of master

* flake8

* typo

* update deprecation warning messages

* remove stub modules from autogenerated api docs

* remove chaco/layers/data

* import from chaco.plots in overlay modules

* import Legend from chaco.overlays.api

Co-authored-by: Xiaoyu Wu <xiaoyu-wu@users.noreply.github.com>
Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>
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.

Consolidate plots into a new "chaco.plots"?
2 participants