-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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.
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
New clear() method for Radio and Check buttons #13401
New clear() method for Radio and Check buttons #13401
Conversation
Ha, I was just trying to get it moving again - I'm not actually familiar enough with the Radio Button stuff to review it. |
Hmmm, well, it was approved, but the tests never passed, hence it never was merged. Its still not passing the tests so its still not going to be merged. People often approve PRs assuming that the tests will catch anything unforeseen. In this case, they appear to have, so you will have to pass that hurdle first. |
I see. What do the tests in Azure pipeline test? what about this one from travis - no language set - are they not the same tests? |
Also, all the errors seem to be with backed, which my code has NOTHING to do with - see https://travis-ci.org/matplotlib/matplotlib/jobs/491327353
|
So when I run the tests locally, this time ensuring there is a DISPLAY via the VNC session, most of the test failures are gone. All the errors in the interactive backend are gone (esp the
|
Looks like the qt5 errors are unrelated. |
So the two errors remaining are to do with
|
Yes @jklymak, these errors seem to be unrelated to my work. What's the process for accepting PRs in a situation like this here? |
I'll try recycling... |
Some (real, this-PR-related) style issues: https://travis-ci.org/matplotlib/matplotlib/jobs/492794388. |
thanks - fixed the two flake issues:
|
Lovely - great to see it all green here :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow numpydoc-style and PEP-257 docstring conventions.
Style comments are addressed. |
Ok, I think I am done. This has been a great learning exercise trying to contribute to a complex OS library :). I totally severely underestimated what it takes :) |
This also closes #10924 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, still some changes needed. But you're getting there.
Thanks Tim for detailed review and comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure that the PR is ready for review to the best of your knowledge and then leave a comment.
Reviewing is a time consuming activity and reviewer time is better spend than finding and commenting on formal and formatting issues. I expect from a PR author to put reasonable care into the PR.
Sorry for being a bit harsh. It's not personal. You are a new contributor, so it's clear that the PR will take more effort, but we're dancing around this single PR for quite some time, and we have a backlog of 270 open PRs in the project.
4e971c7
to
6e60dea
Compare
67ac54f
to
afed741
Compare
afed741
to
c4001e5
Compare
I rebased, cleaned up the conflicts, and squashed to one commit. The I wonder if instead, we want to remember the initial active index set in |
I think I agree that radio buttons should always have at least one element selected. If you need a "none of the above", then that should be another button choice. https://uxplanet.org/radio-buttons-ux-design-588e5c0a50dc |
I'll move to draft |
dcd02b2
to
ef00c97
Compare
OK, |
ef00c97
to
bf16ff0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a what's new entry.
@@ -1844,11 +1901,28 @@ def set_active(self, index): | |||
if self.eventson: | |||
self._observers.process('clicked', self.labels[index].get_text()) | |||
|
|||
def clear(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether this should be named reset()
.
Pro reset
: This is the correct terminology. You cannot clear a radio button group.
Pro clear
: Consistency with other widget. There are a number of widgets with clear
methods, but none with reset
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would probably make more sense, but I left it clear
for consistency.
bf16ff0
to
28116fa
Compare
I’ll review later today, but this looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor docstring improvements.
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com> Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
a21e9b8
to
eb6e233
Compare
PR Summary
Continuing #10924 to clear up some merge/rebase issues and have a fresh start
Fixes #10922
cc @jklymak
PR Checklist