Make ArrowStyle docstrings numpydoc compatible #8343

patniharshit · 2017-03-19T18:36:30Z

NelleV · 2017-03-19T19:19:52Z

lib/matplotlib/patches.py

-              length of the arrow head
+            Parameters
+            ----------
+            head_length : float, optional


can you add the default value as well?

head_length : float, optional, default: 0.4

QuLogic · 2017-03-19T21:45:39Z

lib/matplotlib/patches.py

@@ -3407,8 +3407,7 @@ def transmute(self, path, mutation_size, linewidth):
            return _path, _fillable

    class Curve(_Curve):
-        """
-        A simple curve without any arrow head.
+        """A simple curve without any arrow head.


Are all these single-line changes really necessary? Personally, I don't think they look very good like this.

Yes they don't look good at all. But I think somewhere I saw that for single line docstring.

QuLogic · 2017-03-19T21:45:50Z

lib/matplotlib/patches.py

+            Parameters
+            ----------
+            head_length : float, optional
+                        Length of the arrow head


4-space indent, please.

Sorry for that, I should not have mixed tabs and spaces.

QuLogic · 2017-03-19T21:47:01Z

lib/matplotlib/patches.py

@@ -3520,18 +3524,19 @@ def __init__(self, head_length=.4, head_width=.2):
    _style_list["-|>"] = CurveFilledB

    class CurveFilledAB(_Curve):
-        """
-        An arrow with filled triangle heads both at the begin and the end
+        """An arrow with filled triangle heads both at the begin and the end


This really should be one line. Maybe change to "at both ends" like BracketAB does below.

phobson

Thanks for tackling this, just some minor comments and discussion

phobson · 2017-03-21T03:43:49Z

lib/matplotlib/patches.py


-            *head_width*
-              width of the arrow head
+            head_width : float, optional, default : .2


For all of these, the numbers need a leading zero before the decimal (e.g., ".2" -> "0.2")

phobson · 2017-03-21T03:47:43Z

lib/matplotlib/patches.py

-              length of the arrow head
+            Parameters
+            ----------
+            head_length : float, optional, default : .4


cc @NelleV

I don't we should have two colons in the same sentence. It looks weird to me (but it's not a deal breaker). I think this should read:

headhead_length : float, optional (default is 0.4)

The other question is if these parameters are actually optional. What I mean by that is if you said head_length=None, would this still work by falling back to the default value? If not, then the parameter isn't actually, optional, just named.

Optional does not imply that passing None does the default; it implies that not passing it uses the default.

@QuLogic

You are correct. From the numpydoc spec:

If it is not necessary to specify a keyword argument, use optional:

x : int, optional
Optional keyword parameters have default values, which are displayed as part of the function signature. They can also be detailed in the description:

Description of parameter `x` (the default is -1, which implies summation over all axes).

(though I wish the meaning of "optional" in this context was a little more precise.

Should I change the way defaults are shown then?

That would be my preference, but I'm not going to hold up this PR for that.

QuLogic · 2017-03-21T03:56:45Z

lib/matplotlib/patches.py

+            Parameters
+            ----------
+            head_length : float, optional, default : .4
+                        Length of the arrow head


This should be indented only 4 more spaces from the previous line.

Here only or everywhere?

everywhere. Should like:

... Parameters ---------- head_length : float, optional, default : 0.4 Length of the arrow head ...

dstansby · 2017-03-22T12:51:54Z

lib/matplotlib/patches.py


-            *lengthA*
-              length of the bracket
+            lengthA : int


I'm slightly confused, __init__ just above this docstring doesn't take a lengthA or lengthB argument, is the docstring just wrong here at the moment?

I think you're right @dstansby .

@patniharshit can you update this docstring to more accurately reflect the call signature?

dstansby

Thanks for doing this! Looks good apart from my one comment (which may just be me being confused), will leave it to reviewer 2 to decide what to do about that.

no longer relevant

phobson · 2017-03-22T23:38:33Z

lib/matplotlib/patches.py


-            *lengthA*
-              length of the bracket
+            lengthA : int


I think you're right @dstansby .

@patniharshit can you update this docstring to more accurately reflect the call signature?

dstansby

Thanks a lot for this, and sorry for the many comments! Looks 👍 now

patniharshit · 2017-03-24T10:24:03Z

Happy to contribute :)

Changes made,

Make ArrowStyle docstrings numpydoc compatible

dstansby · 2017-03-26T15:55:06Z

Backported to 2.0.x via. 36ec04f

Make ArrowStyle docstrings numpydoc compatible

bd1e4d7

NelleV reviewed Mar 19, 2017

View reviewed changes

QuLogic previously requested changes Mar 19, 2017

View reviewed changes

patniharshit added 2 commits March 20, 2017 10:46

add default values and remove mixture of tabs and spaces

0009035

add 0 with decimals

b8ae472

phobson previously requested changes Mar 21, 2017

View reviewed changes

QuLogic reviewed Mar 21, 2017

View reviewed changes

change indentation of parameter description

2a31cca

dstansby reviewed Mar 22, 2017

View reviewed changes

dstansby approved these changes Mar 22, 2017

View reviewed changes

phobson requested changes Mar 22, 2017

View reviewed changes

update docstring to reflect the call signature

389d85b

tacaswell added this to the 2.0.1 (next bug fix release) milestone Mar 23, 2017

dstansby approved these changes Mar 24, 2017

View reviewed changes

dstansby added the Documentation label Mar 24, 2017

dstansby changed the title ~~Make ArrowStyle docstrings numpydoc compatible~~ [MRG+1] Make ArrowStyle docstrings numpydoc compatible Mar 24, 2017

phobson approved these changes Mar 24, 2017

View reviewed changes

NelleV changed the title ~~[MRG+1] Make ArrowStyle docstrings numpydoc compatible~~ [MRG+2] Make ArrowStyle docstrings numpydoc compatible Mar 24, 2017

QuLogic changed the title ~~[MRG+2] Make ArrowStyle docstrings numpydoc compatible~~ Make ArrowStyle docstrings numpydoc compatible Mar 24, 2017

QuLogic added the Needs backport label Mar 24, 2017

QuLogic merged commit fc547f4 into matplotlib:master Mar 24, 2017

patniharshit deleted the arrowStyleDocstring branch March 24, 2017 19:59

dstansby pushed a commit that referenced this pull request Mar 26, 2017

Merge pull request #8343 from patniharshit/arrowStyleDocstring

36ec04f

Make ArrowStyle docstrings numpydoc compatible

dstansby removed the Needs backport label Mar 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ArrowStyle docstrings numpydoc compatible #8343

Make ArrowStyle docstrings numpydoc compatible #8343

patniharshit commented Mar 19, 2017

NelleV Mar 19, 2017

patniharshit Mar 20, 2017

QuLogic Mar 19, 2017

patniharshit Mar 20, 2017

QuLogic Mar 19, 2017

patniharshit Mar 20, 2017

QuLogic Mar 19, 2017

phobson left a comment

phobson Mar 21, 2017

patniharshit Mar 21, 2017

phobson Mar 21, 2017

QuLogic Mar 21, 2017

phobson Mar 21, 2017 •

edited

Loading

patniharshit Mar 21, 2017

phobson Mar 22, 2017

QuLogic Mar 21, 2017

patniharshit Mar 21, 2017

phobson Mar 21, 2017 •

edited

Loading

dstansby Mar 22, 2017

phobson Mar 22, 2017

dstansby left a comment

phobson Mar 22, 2017

dstansby left a comment

patniharshit commented Mar 24, 2017

dstansby commented Mar 26, 2017

Make ArrowStyle docstrings numpydoc compatible #8343

Make ArrowStyle docstrings numpydoc compatible #8343

Conversation

patniharshit commented Mar 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phobson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phobson Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phobson Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstansby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstansby left a comment

Choose a reason for hiding this comment

patniharshit commented Mar 24, 2017

dstansby commented Mar 26, 2017

phobson Mar 21, 2017 •

edited

Loading

phobson Mar 21, 2017 •

edited

Loading