Numpydocify RectangleSelector docstring. #17474
Conversation
| @@ -1930,61 +1930,59 @@ def toggle_selector(event): | |||
| _shape_klass = Rectangle | |||
|
|
|||
| def __init__(self, ax, onselect, drawtype='box', | |||
| minspanx=None, minspany=None, useblit=False, | |||
There was a problem hiding this comment.
There's a check for self.minspanx is not None, so I'm not sure if this should change.
There was a problem hiding this comment.
that's fine, if it's None then the test spanx < minspanx is skipped, but if it's zero then spanx < minspanx is never true, so the end result is the same.
lib/matplotlib/widgets.py
Outdated
| The parent axes for the widget. | ||
| onselect : function | ||
| When a selection is made, the selector is cleared and *onselect* | ||
| is called with two arguments: the button press event and button |
There was a problem hiding this comment.
I'm not seeing it in our documentation guide, but don't we have some more standard way of defining these callback signatures?
There was a problem hiding this comment.
may be nice, but I don't know?
lib/matplotlib/widgets.py
Outdated
| backend). | ||
| lineprops : dict, default: \ | ||
| dict(color="black", linestyle="-", linewidth=2, alpha=0.5) | ||
| Properties with which the line is drawn. |
There was a problem hiding this comment.
Only used if using drawtype='line', i.e., not for edges of the rectangle, so I think that should be called out here.
lib/matplotlib/widgets.py
Outdated
| Properties with which the line is drawn. | ||
| rectprops : dict, default: \ | ||
| dict(facecolor="red", edgecolor="black", alpha=0.2, fill=True) | ||
| Properties with which the rectangle is drawn. |
There was a problem hiding this comment.
Similarly, only for drawtype='box'.
lib/matplotlib/widgets.py
Outdated
| spancoords : {"data", "pixels"} | ||
| Whether to interpret *minspanx* and *minspany* in data or in pixel | ||
| coordinates. | ||
| button : `.MouseButton`, list of `.MouseButton` or None, default: None |
There was a problem hiding this comment.
| button : `.MouseButton`, list of `.MouseButton` or None, default: None | |
| button : `.MouseButton`, list of `.MouseButton`, or None, default: None |
lib/matplotlib/widgets.py
Outdated
| button : `.MouseButton`, list of `.MouseButton`, or None, default: None | ||
| Button(s) that trigger rectangle selection; None means "any | ||
| button". |
There was a problem hiding this comment.
| button : `.MouseButton`, list of `.MouseButton`, or None, default: None | |
| Button(s) that trigger rectangle selection; None means "any | |
| button". | |
| button : `.MouseButton` or list of `.MouseButton`, default: any button | |
| The mouse button(s) that control the selection. |
There was a problem hiding this comment.
Not sure about that, as not mentioning None might imply that it means no buttons.
There was a problem hiding this comment.
agree with @QuLogic
There was a problem hiding this comment.
I argue that in this context None is only used as "parameter not specified" = "use the default.
From https://matplotlib.org/devdocs/devel/documenting_mpl.html#default-values:
If None is only used as a sentinel value for "parameter not specified", do not document it as the default. Depending on the context, give the actual default, or mark the parameter as optional if not specifying has no particular effect.
I don't think users should ever explicitly pass None, which as @QuLogic indicated could have a totally different semantic from what it actually does. Therefore I wouldn't even bother documenting it.
That said, it would be much clearer if the value was 'all' instead of None.
There was a problem hiding this comment.
There's plenty of places where None means "all buttons", we're basically stuck with it. I undocumented None.
- The given signature for `onselect` was wrong -- it is called with the start and end events as arguments. - Changed the default of `minspanx`, `minspany` to 0 -- it is easier to document. None is kept as undocumented backcompat; deprecating None would also beed to cover assignment to the `minspanx/y` attributes (probably using a property, yada yada). - `marker_props` is currently ignored, but let's not deprecate it as it would actually make sense to support it (consistently with PolygonSelector -- which names it `markerprops` but heh...).
onselectwas wrong -- it is called with thestart and end events as arguments.
minspanx,minspanyto 0 -- it is easier todocument. None is kept as undocumented backcompat; deprecating None
would also beed to cover assignment to the
minspanx/yattributes(probably using a property, yada yada).
marker_propsis currently ignored, but let's not deprecate it as itwould actually make sense to support it (consistently with
PolygonSelector -- which names it
markerpropsbut heh...).PR Summary
PR Checklist