search

reconhub / incidence

☣:chart_with_upwards_trend::chart_with_downwards_trend:☣ Compute and visualise incidence
https://reconhub.github.io/incidence
Other
58 stars 13 forks source link

Disallow character inputs to incidence() #90

Closed jrcpulliam closed 5 years ago

jrcpulliam commented 5 years ago

Closes #88. Disallows character inputs to incidence() for the following arguments:

Fixes a bug where incidence() was returning an object of class Date when dates argument was character. Strict treatment of character values is intended to help prevent silent errors.

zkamvar commented 5 years ago

Hi,

I’m currently on leave for a family emergency until Mid-January. I’ll have a look at this then.

Sent from my iPhone

On Dec 12, 2018, at 01:41, Juliet Pulliam notifications@github.com wrote:

Closes #88. Disallows character inputs to incidence() for the following arguments:

dates first_date last_date Fixes a bug where incidence() was returning an object of class Date when dates argument was character. Strict treatment of character values is intended to help prevent silent errors.

You can view, comment on, or merge this pull request online at:

https://github.com/reconhub/incidence/pull/90

Commit Summary

fix dispatching and create error message as per #88 add test for the two outcomes modify check_dates to not allow character input incidence.default issues errors for unkown formats; for characters, reminds to convert to Date allow incidence.default to run check_dates to give more specific error messages for unaccepted classes that are dealt with there add tests for error messages generated by incidence.default stricter handling of last_date and first_date; throws an error for any character value add and update tests for stricter treatment of last_date and first_date; not passing test on line 112 of test-incidence.R (expected error not generated) now passing all tests File Changes

M R/check_boundaries.R (6) M R/check_dates.R (8) M R/incidence.R (3) M tests/testthat/test-incidence.R (70) M tests/testthat/test-non-exported.R (4) Patch Links:

https://github.com/reconhub/incidence/pull/90.patch https://github.com/reconhub/incidence/pull/90.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

thibautjombart commented 5 years ago

Looking good. Will check into more details today. Many thanks! @zkamvar this is because as.Date("01-01-01") does not do the job, but does not issue an error either.

thibautjombart commented 5 years ago

Awesome, thanks! And welcome to the team ;)

jrcpulliam commented 5 years ago

My pleasure, and thanks!