jimmyday12 / fitzRoy

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

handle for edge cases in replace_teams #215 #216

Closed JaseZiv closed 4 months ago

JaseZiv commented 4 months ago

Local check produced two failed tests that have nothing to do with the changes proposed in this PR:

══ Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-fetch-betting-odds.R:30:5'): fetch_betting_odds_footywire: works with different inputs  ──
<purrr_error_indexed/rlang_error/error/condition>
Error in `purrr::map(., fetch_betting_odds_page)`: i In index: 1.
Caused by error in `open.connection()`:
! Timeout was reached: [www.footywire.com] SSL connection timeout
Backtrace:
     ▆
  1. ├─... %>% suppressWarnings() at test-fetch-betting-odds.R:30:4
  2. ├─base::suppressWarnings(.)
  3. │ └─base::withCallingHandlers(...)
  4. ├─testthat::expect_warning(.)
  5. │ └─testthat:::expect_condition_matching(...)
  6. │   └─testthat:::quasi_capture(...)
  7. │     ├─testthat (local) .capture(...)
  8. │     │ └─base::withCallingHandlers(...)
  9. │     └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
 10. ├─fitzRoy::fetch_betting_odds_footywire(...)
 11. │ └─... %>% purrr::map(convert_to_data_frame)
 12. ├─purrr::map(., convert_to_data_frame)
 13. │ └─purrr:::map_("list", .x, .f, ..., .progress = .progress)
 14. │   └─purrr:::vctrs_vec_compat(.x, .purrr_user_env)
 15. ├─purrr::map(., ~do.call(extract_table_rows, .))
 16. │ └─purrr:::map_("list", .x, .f, ..., .progress = .progress)
 17. │   └─purrr:::vctrs_vec_compat(.x, .purrr_user_env)
 18. ├─purrr::map(., fetch_betting_odds_page)
 19. │ └─purrr:::map_("list", .x, .f, ..., .progress = .progress)
 20. │   ├─purrr:::with_indexed_errors(...)
 21. │   │ └─base::withCallingHandlers(...)
 22. │   ├─purrr:::call_with_cleanup(...)
 23. │   └─fitzRoy (local) .f(.x[[i]], ...)
 24. │     └─... %>% list(., season)
 25. ├─xml2::read_html(.)
 26. ├─xml2:::read_html.default(.)
 27. │ ├─base::suppressWarnings(...)
 28. │ │ └─base::withCallingHandlers(...)
 29. │ ├─xml2::read_xml(x, encoding = encoding, ..., as_html = TRUE, options = options)
 30. │ └─xml2:::read_xml.character(...)
 31. │   └─xml2:::read_xml.connection(...)
 32. │     ├─base::open(x, "rb")
 33. │     └─base::open.connection(x, "rb")
 34. └─base::.handleSimpleError(...)
 35.   └─purrr (local) h(simpleError(msg, call))
 36.     └─cli::cli_abort(...)
 37.       └─rlang::abort(...)
── Error ('test-fetch-betting-odds.R:137:3'): round numbers don't increment across bye weeks without matches ──
Error in `open.connection(x, "rb")`: Timeout was reached: [www.footywire.com] SSL connection timeout
Backtrace:
     ▆
  1. ├─testthat::expect_warning(...) at test-fetch-betting-odds.R:137:2
  2. │ └─testthat:::expect_condition_matching(...)
  3. │   └─testthat:::quasi_capture(...)
  4. │     ├─testthat (local) .capture(...)
  5. │     │ └─base::withCallingHandlers(...)
  6. │     └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
  7. ├─fitzRoy:::get_footywire_betting_odds(2019, 2020)
  8. │ └─fitzRoy::fetch_betting_odds_footywire(...)
  9. │   └─fitzRoy (local) fetch_valid_seasons()
 10. │     └─fitzRoy (local) fetch_betting_odds_page(2010)
 11. │       └─... %>% list(., season)
 12. ├─xml2::read_html(.)
 13. └─xml2:::read_html.default(.)
 14.   ├─base::suppressWarnings(...)
 15.   │ └─base::withCallingHandlers(...)
 16.   ├─xml2::read_xml(x, encoding = encoding, ..., as_html = TRUE, options = options)
 17.   └─xml2:::read_xml.character(...)
 18.     └─xml2:::read_xml.connection(...)
 19.       ├─base::open(x, "rb")
 20.       └─base::open.connection(x, "rb")
jimmyday12 commented 4 months ago

Thanks @JaseZiv.

I'm going to approve this for now but make a note that I really want to review how this works across the various sources. For example - I'm not super keen to keep the "Western Bulldogs" being converted to Footscray anymore (that was a decision made at the start of the package).

I need to have a think about how to do that since any changes will be breaking changes for people and so will have to transition that change through a few versions.

JaseZiv commented 4 months ago

Thanks @JaseZiv.

I'm going to approve this for now but make a note that I really want to review how this works across the various sources. For example - I'm not super keen to keep the "Western Bulldogs" being converted to Footscray anymore (that was a decision made at the start of the package).

I need to have a think about how to do that since any changes will be breaking changes for people and so will have to transition that change through a few versions.

No probs at all.

I totally agree, personally would've loved to move away from "Footscray" but also didn't fancy causing breaking changes in this PR.

Shout out if you'd like me to help out any further.

Jase