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

Add Season Inputs to flea api calls #408

Merged
merged 10 commits into from Aug 13, 2023

Conversation

RandalMorris
Copy link
Contributor

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.

Copy link
Member

@tanho63 tanho63 left a 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?

R/flea_franchises.R Outdated Show resolved Hide resolved
RandalMorris and others added 2 commits August 13, 2023 08:33
Co-authored-by: Tan Ho <38083823+tanho63@users.noreply.github.com>
@RandalMorris
Copy link
Contributor Author

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.

@@ -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,
Copy link
Member

@tanho63 tanho63 Aug 13, 2023

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)
Copy link
Member

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

Copy link
Member

@tanho63 tanho63 left a 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.

@tanho63 tanho63 enabled auto-merge (squash) August 13, 2023 19:09
@tanho63 tanho63 disabled auto-merge August 13, 2023 19:14
@tanho63 tanho63 merged commit 519915b into ffverse:main Aug 13, 2023
1 of 7 checks passed
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