Issue #809 - Replace is.logical by isTRUE

epiforecasts / scoringutils

Utilities for Scoring and Assessing Predictions

https://epiforecasts.io/scoringutils/

Other

47 stars 19 forks source link

Issue #809 - Replace is.logical by isTRUE #815

Closed nikosbosse closed 1 month ago

nikosbosse commented 1 month ago

Description

This PR closes #809.

Doe what it says on the tin. Purpose is improving robustness and code clarity.

Checklist

[x] My PR is based on a package issue and I have explicitly linked it.
[x] I have included the target issue or issues in the PR title as follows: issue-number: PR title
[x] I have tested my changes locally.
[x] I have added or updated unit tests where necessary.
[x] I have updated the documentation if required.
[x] I have built the package locally and run rebuilt docs using roxygen2.
[x] My code follows the established coding standards and I have run lintr::lint_package() to check for style issues introduced by my changes.
[ ] ~I have added a news item linked to this PR.~
[ ] I have reviewed CI checks for this PR and addressed them as far as I am able.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.54%. Comparing base (fbfbe56) to head (e62a20a).

:exclamation: Current head e62a20a differs from pull request most recent head 7f26674

Please upload reports for the commit 7f26674 to get more accurate results.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #815 +/- ## ======================================= Coverage 95.54% 95.54% ======================================= Files 21 21 Lines 1570 1573 +3 ======================================= + Hits 1500 1503 +3 Misses 70 70 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nikosbosse commented 1 month ago

I think historically we didn't enforce r 3.5 or something like that. Or I was stupid:D

On Sun, 19 May 2024, 15:04 James Azam, @.***> wrote:

@.**** commented on this pull request.

In R/check-input-helpers.R https://github.com/epiforecasts/scoringutils/pull/815#discussion_r1606028651 :

return(is.logical(check))

return(isTRUE(check))

Mmhhh interesting! Why was is.logical() use here? it returns TRUE for both TRUE and FALSE whereas isTRUE will only returns TRUE for a value of TRUE.

— Reply to this email directly, view it on GitHub https://github.com/epiforecasts/scoringutils/pull/815#pullrequestreview-2065075233, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJBYFLLMQFPSQP4I4AWLSQTZDCPOLAVCNFSM6AAAAABH6EUGM6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANRVGA3TKMRTGM . You are receiving this because you authored the thread.Message ID: @.***>

jamesmbaazam commented 1 month ago

I think historically we didn't enforce r 3.5 or something like that. Or I was stupid:D … On Sun, 19 May 2024, 15:04 James Azam, @.> wrote: @*.*** commented on this pull request. ------------------------------ In R/check-input-helpers.R <#815 (comment)> : > - return(is.logical(check)) + return(isTRUE(check)) Mmhhh interesting! Why was is.logical() use here? it returns TRUE for both TRUE and FALSE whereas isTRUE will only returns TRUE for a value of TRUE. — Reply to this email directly, view it on GitHub <#815 (review)>, or unsubscribe <github.com/notifications/unsubscribe-auth/AJBYFLLMQFPSQP4I4AWLSQTZDCPOLAVCNFSM6AAAAABH6EUGM6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANRVGA3TKMRTGM> . You are receiving this because you authored the thread.Message ID: @.>

I think it is unlikely to have caused any bugs unless in edge cases that I can't imagine atm, since "check" is either a string or TRUE. But the isTRUE() replace here is safer.

nikosbosse commented 1 month ago

Lovely, thank you!