Added "val" attribute to widgets.RadioButtons #3971
Conversation
|
Thanks, looks good! Can you squash this down to one commit? I don't think there are currently any tests of the this widget, but if you want to take a crack at adding them it would be awesome (check out the selector tests for a template). |
|
Sounds good. But I am a new to using github. How do I make it just one commit? Do I need On Mon, Jan 5, 2015 at 7:03 PM, Thomas A Caswell notifications@github.com
Daniel D. Hickstein |
|
It is best-practice to make changes on a branch other than master. To squash you need to do an 'interactive rebase'. How familiar with git are you and how patient are you feeling tonight? ;) The very short version of how to do this is to change to the directory where have the repo checked out and do where I am assuming that the remote 'upstream' points towards the canonical mpl repo, and follow the on-screen directions. If you use a GUI to interact with git any reasonable one should have the ability to do this hidden somewhere. You then do a: where I am assuming that the remote 'origin' is your mpl repo on github (you should try this command without the force to see what it does, in general |
| @@ -627,6 +627,9 @@ class RadioButtons(AxesWidget): | |||
|
|
|||
| *circles* | |||
| A list of :class:`matplotlib.patches.Circle` instances | |||
|
|
|||
| *val* | |||
There was a problem hiding this comment.
FWIW - I don't see a good reason not to be very explicit about the name of the variable. value would be good, selected_value would be even better...
|
Could you initialize the attribute to None at the beginning of the constructor? It is technically possible for the attribute to be undefined if one specifies an "active" value that is outside the range of the list. As for the name of the attribute, there is no need to stick with the other widget's conventions. I personally vote for "selected_label" in order to emphasize that you get back a string, not an index. As a side note, I will be making a bunch of little PRs tonight, and one of them will be adding a setter method to this class. |
|
In fact, do we want to go all the way with this and make the attribute a full-fledged property? |
|
Darn you @tacaswell, first you make me learn how look at the mpl source code, then you make me learn how to use github, then you make me learn this rebase nonsense (which required that I learn how to use vim). Will the learning ever stop?? :) Anyway, I think that I squashed this to a single commit. I also changed "val" to "value_selected" as suggested. I don't really know what is involved in making this property versus an attribute, but it seems like a good idea to be able to set the status of any of the widgets (for example, someone may want to have a button that resets all of the other buttons to their default values. If I understand correctly, this does not appear to be possible with the current radioButtons. |
|
The value of making it a property rather than an attribute is that you can make it 'read only' by not defining a setter, thus will raise an exception. If you do define a setter on the property you can validate the assigned value on the way in and update the gui to reflect the changes. |
Overkill? There is nothing that is truly read only, and what is the harm in somebody modifying their event's selected_value if they really want to do so?
For me, this is the only critical thing for this PR, but I'm happy if @tacaswell really wants this to become a property. |
|
Don't worry about making it a property, I can handle that if we want to (my On Thu, Jan 8, 2015 at 5:30 PM, Phil Elson notifications@github.com wrote:
|
|
@pelson I was just babbling about what you can do with properties not saying that it should be done, sorry should have been more clear. The second usage (the validation) seems more useful anyway as now changing selected and update the GUI as well. |
|
In the most recent commit, I included the initialization to None at the beginning, so is the current state of the PR fine? |
|
I am +1 now |
ENH : Added "value_selected" attribute to widgets.RadioButtons
|
@DanHickstein Congratulations on your first mpl contribution! Hopefully it is the first of many. |
|
Awesome! Thanks for walking me through it :) |
Baptism of fire 🔥 🚒 ? 😉 Congratulations! |
To ensure backcompat without bothering the majority of users who don't actually access the .circles attribute, dynamically (and irreversibly) switch back to the old draw method (list of Circles) whenever that attribute is accessed for the first time (if ever).
It would be nice to be able to query the current selection of a set of RadioButtons using "rButton.val", just like one can query the current value of a slider using "mySlider.val". I included two lines of code that allow this to be done.