DOC - SpanSelector widget documentation #7047

Merged
merged 2 commits into from Sep 9, 2016

Conversation

Projects
None yet
7 participants

LindyBalboa commented Sep 6, 2016 edited

Initial focus was to add a reference to AxesWidget.active in the documentation. I went ahead and made it numpy standard while I was in there.

Removed: I made the ignore method into _ignore so that it would not be build into the docs, as a user will have zero reason to call it.

Resolves #7009

mdboom added the needs_review label Sep 6, 2016

LindyBalboa changed the title from Issue 7009 - SpanSelector documentation to DOC - SpanSelector widget documentation Sep 6, 2016

@NelleV NelleV commented on an outdated diff Sep 6, 2016

lib/matplotlib/widgets.py
- If *minspan* is not *None*, ignore events smaller than *minspan*
+ onmove_callback : function of form func(min, max), where min/max are floats, default is None
@NelleV

NelleV Sep 6, 2016

Contributor

I am pretty sure this line is too long for the pep8 requirements (to be confirmed when travis finish running).

@NelleV NelleV commented on an outdated diff Sep 6, 2016

lib/matplotlib/widgets.py
- If *span_stays* is True, the span stays visble after making
- a valid selection.
+ Examples
+ --------
+ >>> import matplotlib.pyplot as plt
+ >>> import matplotlib.widgets as mwidgets
+ >>> fig, ax = plt.subplots()
+ >>> ax.plot([1,2,3],[10,50,100])
@NelleV

NelleV Sep 6, 2016

Contributor

PEP8: there are spaces missing after the comas.

Contributor

NelleV commented Sep 6, 2016

I am slightly confused on why the pep8 test pass here, while they don't for all of my students :(
Two lines are two long for our pep8 requirements:

widgets.py|1710 col 80| E501 line too long (96 > 79 characters)
widgets.py|1732 col 80| E501 line too long (89 > 79 characters)

Apart from this, and my additional comment, the patch looks good!

Thanks,
N

Member

QuLogic commented Sep 6, 2016

The changed file is listed in the known bad files list, so any PEP8 issues in it get ignored.

If your students wish to correct these issues, that would be much appreciated.

@QuLogic QuLogic and 1 other commented on an outdated diff Sep 6, 2016

lib/matplotlib/widgets.py
@@ -1794,7 +1799,7 @@ def new_axes(self, ax):
self.ax.add_patch(self.rect)
self.artists = [self.rect]
- def ignore(self, event):
+ def _ignore(self, event):
@QuLogic

QuLogic Sep 6, 2016 edited

Member

I'm not sure you should be changing the name here. Surely there are other callers that should be updated as well?

@LindyBalboa

LindyBalboa Sep 7, 2016

You are totally right. I thought I changed where it is called as well. I will take another look.

LindyBalboa commented Sep 7, 2016 edited

I can't seem to do a linebreak with doctest line 1732 and make it work. And I feel making that line shorter would just muddle it and look poor. It turns out there is in fact an example for SpanSelector in the examples folder, but since it is interactive, telling it to plot to the docs doesn't make much sense.

Should I just make a link to the example, and if so, how would I make that link?

Contributor

NelleV commented Sep 7, 2016

I think the following is fine::

>>> span = mwidgets.SpanSelector(ax, onselect, 'horizontal', 
                                 rectprops=rectprops)
LindyBalboa Clean up SpanSelector documentation
Add reference to AxesWidget.active for disabling the selector.

Resolves #7009
f1c72a8

So, Travis is giving this:

ValueError: Some exclude patterns were unnecessary as the files they pointed to either passed the PEP8 tests or do not point to a file:
E                 */matplotlib/widgets.py

Does that mean it can be removed from the list or am I missing something?

Member

Kojoley commented Sep 8, 2016

It seems you have fixed all pep8 problems in widgets.py, so yes, you should remove it from the pep8 exclusion list (this line in test_coding_standards.py).

LindyBalboa Remove widgets.py from PEP8 excetions
3082e90
Contributor

NelleV commented Sep 8, 2016

The failure on travis is unrelated. LGTM 👍

Owner

jenshnielsen commented Sep 8, 2016

Failure is unrelated. Restarted the particular travis job to be sure

@tacaswell tacaswell merged commit ed9711d into matplotlib:master Sep 9, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 70.362%
Details

tacaswell removed the needs_review label Sep 9, 2016

@tacaswell tacaswell added a commit that referenced this pull request Sep 9, 2016

@tacaswell tacaswell Merge pull request #7047 from LindyBalboa/issue_7009
DOC: SpanSelector widget documentation
a2e56ad
Owner

tacaswell commented Sep 9, 2016

backported to v2.x as a2e56ad

LindyBalboa deleted the LindyBalboa:issue_7009 branch Sep 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment