X Tutup
Skip to content

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

Merged
tacaswell merged 5 commits intomatplotlib:masterfrom
devashishd12:param-fix
Dec 22, 2015
Merged

Addresses issue #5704. Makes usage of parameters clearer#5709
tacaswell merged 5 commits intomatplotlib:masterfrom
devashishd12:param-fix

Conversation

@devashishd12
Copy link
Copy Markdown
Contributor

@devashishd12 devashishd12 commented Dec 21, 2015

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

@devashishd12
Copy link
Copy Markdown
Contributor Author

devashishd12 commented Dec 21, 2015

@tacaswell please have a look.

@tacaswell
Copy link
Copy Markdown
Member

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?

tacaswell
tacaswell reviewed Dec 22, 2015
View reviewed changes
lib/matplotlib/axes/_axes.py Outdated
Copy link
Copy Markdown
Member

@tacaswell tacaswell Dec 22, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@devashishd12 devashishd12 Dec 22, 2015

Choose a reason for hiding this comment

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

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...

Copy link
Copy Markdown
Member

@tacaswell tacaswell Dec 22, 2015

Choose a reason for hiding this comment

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

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 tacaswell added this to the next major release (2.0) milestone Dec 22, 2015
@devashishd12
Copy link
Copy Markdown
Contributor Author

devashishd12 commented Dec 22, 2015

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

@tacaswell
Copy link
Copy Markdown
Member

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).

@devashishd12
Copy link
Copy Markdown
Contributor Author

devashishd12 commented Dec 22, 2015

Oh alright thanks!

tacaswell added a commit that referenced this pull request Dec 22, 2015
@tacaswell
Merge pull request #5709 from dsquareindia/param-fix
7868f0b 
DOC:  clarify annotation parameter usage

#5704
@tacaswell tacaswell merged commit 7868f0b into matplotlib:master Dec 22, 2015
@tacaswell
Copy link
Copy Markdown
Member

tacaswell commented Dec 22, 2015

Thanks!

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

tacaswell added a commit that referenced this pull request Dec 22, 2015
@tacaswell
Merge pull request #5709 from dsquareindia/param-fix
82b29ae 
DOC:  clarify annotation parameter usage

#5704
@tacaswell
Copy link
Copy Markdown
Member

tacaswell commented Dec 22, 2015

backported to 1.5.x as 82b29ae

@QuLogic QuLogic modified the milestones: Critical bugfix release (1.5.1), next major release (2.0) Dec 22, 2015
@devashishd12 devashishd12 deleted the param-fix branch December 23, 2015 11:22
@devashishd12
Copy link
Copy Markdown
Contributor Author

devashishd12 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

Reviewers

No reviews

Assignees

No one assigned

Labels

Documentation

Projects

None yet

Milestone

v1.5.1

Development

Successfully merging this pull request may close these issues.

3 participants

@devashishd12 @tacaswell @QuLogic
X Tutup