-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Start Yahoo Implementation #336 #424
Conversation
R/yahoo_franchises.R
Outdated
franchise_name <- xml2::xml_text(xml2::xml_find_all(xml_doc, "//team/name")) | ||
user_name <- xml2::xml_text(xml2::xml_find_all(xml_doc, "//manager/nickname")) | ||
user_id <- xml2::xml_text(xml2::xml_find_all(xml_doc, "//manager/guid")) | ||
co_owners <- NA # This is available in Yahoo but I'm bad at R so I can't tell what format it's supposed to be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What format should the co_owners field be in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Generally have provided just the co-manager IDs in the past as a nested vector, can do that here or provide more of these details collapsed into a nested list/tibble.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Studying the example provided here and I'm not sure your above code will work as intended because xml_find_all will strip every user (manager or co-manager) into a plain vector.
This might result in inaccurate output when combined into a dataframe later (i.e. if franchise_name is a vector of 12 names and user_name is a vector of 16 names, combining these back into a base R dataframe would result in recycling the shorter vector to match the longer one)
Can assist further if you provide the full XML output from xml_doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Hadn't tested with co_owners yet. I updated it to support co_owners as a nested list. I also did add an example xml_file tests/yahoo_example_responses/single-league-teams.xml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to see someone take this on! I've added a feature branch yahoo
which we can use to merge reviewed progress to (so that any other platform PRs don't affect this one).
Have left a few comments, feel free to reply here or take it to discord DMs for discussions if preferred
R/yahoo_franchises.R
Outdated
franchise_name <- xml2::xml_text(xml2::xml_find_all(xml_doc, "//team/name")) | ||
user_name <- xml2::xml_text(xml2::xml_find_all(xml_doc, "//manager/nickname")) | ||
user_id <- xml2::xml_text(xml2::xml_find_all(xml_doc, "//manager/guid")) | ||
co_owners <- NA # This is available in Yahoo but I'm bad at R so I can't tell what format it's supposed to be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Generally have provided just the co-manager IDs in the past as a nested vector, can do that here or provide more of these details collapsed into a nested list/tibble.
R/yahoo_franchises.R
Outdated
franchise_name <- xml2::xml_text(xml2::xml_find_all(xml_doc, "//team/name")) | ||
user_name <- xml2::xml_text(xml2::xml_find_all(xml_doc, "//manager/nickname")) | ||
user_id <- xml2::xml_text(xml2::xml_find_all(xml_doc, "//manager/guid")) | ||
co_owners <- NA # This is available in Yahoo but I'm bad at R so I can't tell what format it's supposed to be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Studying the example provided here and I'm not sure your above code will work as intended because xml_find_all will strip every user (manager or co-manager) into a plain vector.
This might result in inaccurate output when combined into a dataframe later (i.e. if franchise_name is a vector of 12 names and user_name is a vector of 16 names, combining these back into a base R dataframe would result in recycling the shorter vector to match the longer one)
Can assist further if you provide the full XML output from xml_doc?
5f22bf8
to
19bf511
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review @tanho63! I tried to address all feedback.
R/yahoo_franchises.R
Outdated
franchise_name <- xml2::xml_text(xml2::xml_find_all(xml_doc, "//team/name")) | ||
user_name <- xml2::xml_text(xml2::xml_find_all(xml_doc, "//manager/nickname")) | ||
user_id <- xml2::xml_text(xml2::xml_find_all(xml_doc, "//manager/guid")) | ||
co_owners <- NA # This is available in Yahoo but I'm bad at R so I can't tell what format it's supposed to be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Hadn't tested with co_owners yet. I updated it to support co_owners as a nested list. I also did add an example xml_file tests/yahoo_example_responses/single-league-teams.xml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I added some test mocks to your PR branch so that we can start using some tests here and added some crude tests to tests/testthat/tests-yahoo.R. Might be easiest to do up the PR, add the tests, and then send me a token to do the mock-test-uploads 😀
Left a few style notes for a more "tidy" way to do this, mostly-optional so if you'd prefer I merge and we tackle that after we get a more complete first implementation let me know
R/yahoo_franchises.R
Outdated
ff_franchises.yahoo_conn <- function(conn) { | ||
glue::glue("leagues;league_keys={conn$league_key}/teams") %>% | ||
yahoo_getendpoint(conn) %>% | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto with above re: getElement etc etc
} | ||
} | ||
|
||
.yahoo_process_franchises_response <- function(xml_doc) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Token you gave me expired before I could fully flesh out this example, but here's an alternative way to approach this problem - convert xml to list and use tidyr/dplyr/purrr to unnest out the dataframe you want
x <- xml_doc %>%
xml2::as_list() %>%
purrr::pluck("fantasy_content", "leagues","league","teams") %>%
tibble::tibble() %>%
tidyr::unnest_wider(1) %>%
dplyr::select(
franchise_id = team_id,
franchise_name = name,
division_id = division_id,
managers
) %>%
tidyr::unnest_longer(c(franchise_id, franchise_name, division_id)) %>%
dplyr::mutate(
managers = purrr::map(managers, ~ tibble::tibble(.x) %>% tidyr::unnest_wider(1))
) %>%
tidyr::hoist(
managers,
"user_name" = "nickname",
"user_id" = "guid",
"user_felo" = "felo_score"
) %>%
tidyr::unnest_longer(c(user_name, user_id, user_felo)) %>%
tidyr::unnest_longer(c(user_name, user_id, user_felo))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent a few hours reading up on the tidyverse but I just can't for the life of me figure out how to work with the as_list xml output. I'm going to go with the xml_find_all solutions for now since they're super intuitive for the uninitated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the uninitated.
the un-tidy-doctrinated 😂
That's fine. Let's put in getElement in all the relevant places and call it the finish line for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe useful future reading if you want to look at unnesting in the future: https://r4ds.hadley.nz/rectangling
Co-authored-by: Tan Ho <[email protected]>
e96f36d
to
07e1d9d
Compare
Starting work on: #336
This PR implements ff_connect, ff_userleagues, and ff_franchises for Yahoo.
To login, users will need to
I built that website for grabbing the auth token as part of a past project https://introductory.medium.com/download-yahoo-fantasy-football-data-bc1e1db7ee49
I plan to finish the Yahoo implementation, but I'm starting small.