Addresses issue #5704. Makes usage of parameters clearer #5709

Merged
merged 5 commits into from Dec 22, 2015

Conversation

Projects
None yet
3 participants
Contributor

dsquareindia commented Dec 21, 2015

Just added a few lines to make the dependence of the different parameters clearer.

@dsquareindia dsquareindia Just added a few lines to make the dependence of the different parame…
…ters clearer.
f7f16fe
Contributor

dsquareindia commented Dec 21, 2015

@tacaswell please have a look.

Owner

tacaswell commented Dec 21, 2015

It looks like there are some pep8 issues:

======================================================================
FAIL: matplotlib.tests.test_coding_standards.test_pep8_conformance_installed_files
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/tests/test_coding_standards.py", line 249, in test_pep8_conformance_installed_files
    expected_bad_files=expected_bad_files)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/tests/test_coding_standards.py", line 143, in assert_pep8_conformance
    assert_equal(result.total_errors, 0, msg)
AssertionError: 3 != 0 : Found code syntax errors (and warnings):
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:636:80: E501 line too long (85 > 79 characters)
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:639:80: E501 line too long (81 > 79 characters)
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:651:80: E501 line too long (82 > 79 characters)
----------------------------------------------------------------------

Can you please wrap those lines to <80 characters?

@dsquareindia dsquareindia Reduced no of characters to <80
8b2d58f

@tacaswell tacaswell and 1 other commented on an outdated diff Dec 22, 2015

lib/matplotlib/axes/_axes.py
@@ -632,22 +632,21 @@ def annotate(self, *args, **kwargs):
s : string
label
- xy : (x, y)
- position of element to annotate
+ xy : (x, y) , default: "data"
@tacaswell

tacaswell Dec 22, 2015

Owner

This does not default to 'data'.

I would add something to the body of this parameter like 'see xycoords to control what coordinate system this value is interpreted in' and something similar for xytext.

@dsquareindia

dsquareindia Dec 22, 2015

Contributor

Oh sorry I'll remove the default value for xy. Yes what you said makes it much clearer but then I think I'll have to use the next line. Is it fine if I do so? Ideally I don't really want to do that though...

@tacaswell

tacaswell Dec 22, 2015

Owner

Use as many lines as you need.

Not everything can be documented in about half a tweet

On Tue, Dec 22, 2015, 01:34 Devashish Deshpande notifications@github.com
wrote:

In lib/matplotlib/axes/_axes.py
#5709 (comment):

@@ -632,22 +632,21 @@ def annotate(self, _args, *_kwargs):
s : string
label

  •    xy : (x, y)
    
  •        position of element to annotate
    
  •    xy : (x, y) , default: "data"
    

Oh sorry I'll remove the default value for xy. Yes what you said makes it
much clearer but then I think I'll have to use the next line. Is it fine if
I do so? Ideally I don't really want to do that though...


Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/5709/files#r48225539.

tacaswell added this to the next major release (2.0) milestone Dec 22, 2015

dsquareindia added some commits Dec 22, 2015

@dsquareindia dsquareindia Removed default value for xy, added details for param use.
fceb695
@dsquareindia dsquareindia eliminated trailing whitespace. Changed the text a bit.
2f97019
@dsquareindia dsquareindia Replaced ` with * emphasis
637b273
Contributor

dsquareindia commented Dec 22, 2015

@tacaswell I'm not able to understand why it's failing now....

Owner

tacaswell commented Dec 22, 2015

Restarted, That particular failure is checking relative speeds, but the tests run on VMs without any performance guarantees so it intermittently fails (see that one maybe once or twice a week).

Contributor

dsquareindia commented Dec 22, 2015

Oh alright thanks!

@tacaswell tacaswell added a commit that referenced this pull request Dec 22, 2015

@tacaswell tacaswell Merge pull request #5709 from dsquareindia/param-fix
DOC:  clarify annotation parameter usage

#5704
7868f0b

@tacaswell tacaswell merged commit 7868f0b into matplotlib:master Dec 22, 2015

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 68.319%
Details

tacaswell removed the needs_review label Dec 22, 2015

Owner

tacaswell commented Dec 22, 2015

Thanks!

Contributions to documentation are as or more important as code contributions. 👍

@tacaswell tacaswell added a commit that referenced this pull request Dec 22, 2015

@tacaswell tacaswell Merge pull request #5709 from dsquareindia/param-fix
DOC:  clarify annotation parameter usage

#5704
82b29ae
Owner

tacaswell commented Dec 22, 2015

backported to 1.5.x as 82b29ae

dsquareindia deleted the dsquareindia:param-fix branch Dec 23, 2015

Contributor

dsquareindia commented Dec 23, 2015

Will surely keep that in mind :-)

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