-
-
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
Add Season Inputs to flea api calls #408
Conversation
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.
Looks good to me! One styling suggestion (which you can implement by pressing commit-suggestion button below and then pulling the branch back down to your machine).
Can you also increment the dev version in the DESCRIPTION (i.e. from 1.4.8.05 to 1.4.8.06) and add a line in NEWS.md that describes what changed?
Co-authored-by: Tan Ho <[email protected]>
Added the changes. I did notice the rosters has a scoring_period input so right now it just pulls in week 1 rosters for a given year. |
R/flea_playerscores.R
Outdated
@@ -25,6 +25,7 @@ ff_playerscores.flea_conn <- function(conn, page_limit = NULL, ...) { | |||
endpoint = "FetchPlayerListing", | |||
sport = "NFL", | |||
league_id = conn$league_id, | |||
season = conn$season, |
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.
seems like this parameter is actually called sort_season
https://www.fleaflicker.com/api-docs/index.html#operation--FetchPlayerListing-get - have updated
@@ -23,6 +23,6 @@ test_that("ff_franchises returns a tibble of franchises", { | |||
expect_tibble(dlf_franchises, nrows = 16) | |||
expect_tibble(jml_franchises, nrow = 12) | |||
expect_tibble(dlp_franchises, nrow = 12) | |||
expect_tibble(joe_franchises, nrow = 12) | |||
expect_tibble(joe_franchises, nrow = 16) |
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.
This actually explains a pesky problem as to why the number of franchises had changed in this test: the league originally had 16 in 2020 but now has 12, and I couldn't figure out why this wasn't working before 😅 1a47994
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.
Thanks for the additions! Have made a few styling things and added the week arg to ff_rosters()
you were mentioning.
Third time is the charm right? lol
These are the API calls that the season parameter can be added too, Without them they default to current season.