-
Notifications
You must be signed in to change notification settings - Fork 55
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
Convenience classes for widget Actions #360
Conversation
Codecov Report
@@ Coverage Diff @@
## master #360 +/- ##
==========================================
+ Coverage 35.15% 36.31% +1.15%
==========================================
Files 463 481 +18
Lines 25820 26397 +577
Branches 3838 3909 +71
==========================================
+ Hits 9076 9585 +509
- Misses 16350 16399 +49
- Partials 394 413 +19
Continue to review full report at Codecov.
|
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 basically 👍 on this. Just adding a few remaining comments now that I've done a more careful reading.
I'm starting to write tests and finding some interesting issues, so there are likely to be some updates on this (currently |
This is being delayed a bit more: dispatch of traits listeners needs to be on the UI thread so I need to refactor away from |
... except UI dispatch requires TraitsUI to be imported, which means tests have errors unless there happens to have been an import of TraitsUI somewhere. I am knee-deep in yak hair right now. |
@jwiggins This has been thoroughly worked over, so a re-review would be appreciated at some point. |
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.
Found a few things looking over the most recent changes. I'm about to do another pass over everything...
It's good that this is covered by tests now. I won't push too hard for big changes, because I'd rather see this merged soon. That said, the repetition in the testing code is a bit of a smell. It would be nice to track that in a separate issue and clean it up later. (I see that |
I think the testing of the field classes is about as tight as you are going to be able to get it, although it would be nice to have the GuiTestAssistant available to make some of the event processing nicer and a bit more robust. The action testing is not as good, but it should really be done as part of re-writing the tests for all the actions. |
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.
Pending a clean CI run, this is good-to-go
Looks good. Merging. |
This PR adds:
Fixes #349