Skip to content
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

Clamp bugfix #2365

Merged
merged 4 commits into from
Nov 28, 2022
Merged

Clamp bugfix #2365

merged 4 commits into from
Nov 28, 2022

Conversation

johrstrom
Copy link
Contributor

@johrstrom johrstrom commented Nov 9, 2022

Fixes #2362 which ended up being a few bugs. I'll leave comments for rationale for changes in the files.

┆Issue is synchronized with this Asana task by Unito
┆Link To Task: https://app.asana.com/0/1135148780858012/1203338277276811

Comment on lines -426 to +427
} else if(previous && previous['max'] && currentValue == previous['max']) {
return next['max'];
} else if(previous && previous['min'] && currentValue == previous['min']) {
} else if(currentValue === previous['min']) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous['max'] can be 0 which evaluates to fasley - so just stop checking for it's existance because javascript deals well with comparing undefined or null against actual objects.

This also makes sure we check the minimum before the maximum so things get clamped to the minimums.

Comment on lines +92 to +97
data-hide-cuda-version: "true",
data-hide-advanced-options: "true",
data-min-bc-num-slots: "100",
data-max-bc-num-slots: "200",
data-min-bc-num-hours: "444",
data-max-bc-num-hours: "555",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These modifications on old tests are just to be sure that when folks use strings instead of integers or booleans everything still works as expected.

Copy link
Contributor

@Oglopf Oglopf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@johrstrom johrstrom merged commit 9e52401 into master Nov 28, 2022
@johrstrom johrstrom deleted the clamp-bugfix branch November 28, 2022 19:33
johrstrom added a commit that referenced this pull request Dec 7, 2022
Fix clamping to and from 0 to behave correctly.
johrstrom added a commit that referenced this pull request Dec 15, 2022
Fix clamping to and from 0 to behave correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clamping to or from 0 seems broken
3 participants