-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[FIX] owheatmap: Prevent sliders to set Low >= High #2025
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2025 +/- ##
========================================
Coverage ? 70.7%
========================================
Files ? 343
Lines ? 54464
Branches ? 0
========================================
Hits ? 38509
Misses ? 15955
Partials ? 0 Continue to review full report at Codecov.
|
self.controls.threshold_low.setSliderPosition(19) | ||
if self.controls.threshold_low.value() >= self.controls.threshold_high.value(): | ||
self.controls.threshold_high.setSliderPosition(self.controls.threshold_low.value() + 1) | ||
self.update_color_schema() |
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 don't like that if someone changes the slider parameters (range or steps) 800 lines higher, the hard-coded constants (20 and 19) here would need to be updated too (easy to miss).
How about avoiding the constants and making the code a bit shorter, with a function like:
def update_lowslider(self):
low, high = self.controls.threshold_low, self.controls.threshold_high
if low.value() >= high.value():
low.setSliderPosition(high.value() - 1)
self.update_color_schema()
And similarly for the highslider below.
@@ -98,3 +98,12 @@ def test_not_enough_data_settings_changed(self): | |||
self.assertFalse(msg.not_enough_features.is_shown()) | |||
self.assertFalse(msg.not_enough_instances.is_shown()) | |||
self.assertFalse(msg.not_enough_instances_k_means.is_shown()) | |||
|
|||
def test_color_low_high(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.
For docstrings use triple double-quotes """
(pep 8, 257)
Looks good to me. You can squash the commits into one and let's merge. |
AssertionError thrown or error message shown if Low >= High. Prevent horizontal sliders to set Low >= High. - [X] Code changes - [X] Tests - [ ] Documentation
Issue
AssertionError thrown or error message shown if Low >= High.
Description of changes
Prevent horizontal sliders to set Low >= High.
Includes