Skip to content
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

handle for edge cases in replace_teams #215 #216

Merged
merged 1 commit into from
May 20, 2024

Conversation

JaseZiv
Copy link
Contributor

@JaseZiv JaseZiv commented May 19, 2024

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
Copy link
Owner

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.

@jimmyday12 jimmyday12 merged commit efcaa93 into jimmyday12:main May 20, 2024
7 checks passed
@JaseZiv
Copy link
Contributor Author

JaseZiv commented May 20, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants