8000 Addresses issue #5704. Makes usage of parameters clearer by devashishd12 · Pull Request #5709 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 22, 2015
Merged

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

merged 5 commits into from
Dec 22, 2015

Conversation

devashishd12
Copy link
Contributor

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

@devashishd12
Copy link
Contributor Author

@tacaswell please have a look.

@tacaswell
Copy link
Member

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
@@ -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"
Copy link
Member

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
Contributor Author

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
Member

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
Contributor Author

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

@tacaswell
Copy link
Member

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
Contributor Author

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
Member

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
Member

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
Contributor Author

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
0