search

jimmyday12 / fitzRoy

A set of functions to easily access AFL data
https://jimmyday12.github.io/fitzRoy
Other
129 stars 27 forks source link

Fix round calculation #92

Closed cfranklin11 closed 4 years ago

cfranklin11 commented 4 years ago

Resolves #93

I came across a few issues with round numbers recently:

  1. I must have missed this earlier, but the date filter for "2018-04-25 15:20:00" doesn't work for betting data, because it has dates only, no match start times. Normalising dates to remove times for the sake of matching fixes the problem.

  2. Requesting multiple seasons for get_footywire_betting_data resulted in incorrect round numbers, if the request included 2014, because it started on week 11, whereas the other valid seasons all started on week 12. Grouping by season before calculating the minimum week number fixed this.

  3. A lot of round numbers were wrong in general due to (I believe) a change in how lubridate calculates weeks & weekdays. I remember looking at the documentation, and it saying that Sunday is always 7 and Monday always 1, but now it's relative to when the week is set to start, with the default being Sunday. This means that by default, Sunday is 1 and Monday is 2. I left the default and changed the round week shift to start on Sunday instead of Monday now.

cfranklin11 commented 4 years ago

I've noticed some testthat::skip_on_cran() in the test files, so let me know if I should add that to new tests. Also, I held off on updating NEWS with the bug fix since you're submitting the package to CRAN.

jimmyday12 commented 4 years ago

I've noticed some testthat::skip_on_cran() in the test files, so let me know if I should add that to new tests. Also, I held off on updating NEWS with the bug fix since you're submitting the package to CRAN.

Hi @cfranklin11 yeah sorry I missed this but CRAN forces you to have a test suite that run in <10s so given our tests usually involve API's/downloading data I've had to skip them all on CRAN. If you could include those it would be great :)

cfranklin11 commented 4 years ago

@jimmyday12 I added the test skipping for CRAN and updated the docs/news, so this should be good to merge

jimmyday12 commented 4 years ago

Looks great thanks @cfranklin11