-
-
Notifications
You must be signed in to change notification settings - Fork 6
Fixes warning on choices to be thrown only after prettyfying them #677
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
base: main
Are you sure you want to change the base?
Conversation
Code Coverage SummaryDiff against mainResults for commit: e92104e Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Tests Summary 1 files 31 suites 37s ⏱️ Results for commit e92104e. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 32edb24 ♻️ This comment has been updated with latest results. |
llrs-roche
left a comment
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.
Looks good. Key change I think is on https://github.com/insightsengineering/teal.slice/pull/677/changes#diff-83c9e316098506afac069a38830850c5a3bcd979de2f8eb540787de4c5af946cR288.
The app didn't produce the warnings with this patch. Thanks for adding the NEWS entry
| ) | ||
| }) | ||
|
|
||
| testthat::describe("constructor modifies choices", { |
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.
Loading main and running the new tests on this branch leds to: ! Test failed with 2 failures and 1 success.:
── Failure: constructor modifies choices / and throws warning when manually calculated with range(x) ──
Expected `RangeFilterState$new(...)` to produce warnings.
── Failure: constructor modifies choices / and does not throw error when re-used ──
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.
Thinking about what prompted me to refer to tests: could we test this with some NA and some Inf and (?) imaginary numbers (check ?Re): 0i ^ (-3:3). Just to be sure if we can further simplify the is.na | !is.finite
Pull Request
Fixes of #676
Changes description