Deprecates year argument in espn_cfb_team_coaches#126
Open
bradisbrad wants to merge 2 commits into
Open
Conversation
7 tasks
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideDeprecates and softens the behavior of the year argument in espn_cfb_team_coaches() to align with ESPN’s current API behavior (only current-season coach is returned), updates its documentation, and slightly cleans up style/tests in related files. Sequence diagram for updated espn_cfb_team_coaches year handlingsequenceDiagram
actor User
participant espn_cfb_team_coaches
participant most_recent_cfb_season
participant cli
participant ESPN_API
User->>espn_cfb_team_coaches: espn_cfb_team_coaches(team_id, year?)
alt [year is missing]
espn_cfb_team_coaches->>most_recent_cfb_season: most_recent_cfb_season()
most_recent_cfb_season-->>espn_cfb_team_coaches: current_year
else [year is provided]
espn_cfb_team_coaches->>most_recent_cfb_season: most_recent_cfb_season()
most_recent_cfb_season-->>espn_cfb_team_coaches: current_year
alt [year != current_year]
espn_cfb_team_coaches->>cli: cli_warn()
espn_cfb_team_coaches->>espn_cfb_team_coaches: year <- current_year
end
end
espn_cfb_team_coaches->>espn_cfb_team_coaches: validate_year(year)
espn_cfb_team_coaches->>ESPN_API: GET /seasons/{year}/teams/{team_id}/coaches
ESPN_API-->>espn_cfb_team_coaches: coach data (current season)
espn_cfb_team_coaches-->>User: tibble with current season coach
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthrough
ChangesCFB team updates
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
espn_cfb_team_coaches(),most_recent_cfb_season()is called multiple times (default arg, comparison, and reassignment); consider caching it in a localcurrent_yearvariable to avoid redundant calls and potential inconsistencies. - This PR introduces several whitespace/indentation-only changes in other team-related functions; it would be cleaner to limit the diff to the coach deprecation changes to keep history focused and make future blame/debugging easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `espn_cfb_team_coaches()`, `most_recent_cfb_season()` is called multiple times (default arg, comparison, and reassignment); consider caching it in a local `current_year` variable to avoid redundant calls and potential inconsistencies.
- This PR introduces several whitespace/indentation-only changes in other team-related functions; it would be cleaner to limit the diff to the coach deprecation changes to keep history focused and make future blame/debugging easier.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/testthat/test-cfbd_player_info.R`:
- Around line 6-7: Update the expanded column assertions in the player-info
tests to verify that the expected fields are a subset of the API response rather
than requiring exact set equality. Apply this change to the assertions around
the listed columns and the corresponding lines 26–28, preserving all required
field names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbc43868-56f3-4486-8cd2-7891dcbd6950
📒 Files selected for processing (3)
R/espn_cfb_team.Rman/espn_cfb_team_coaches.Rdtests/testthat/test-cfbd_player_info.R
Comment on lines
+6
to
+7
| "home_town", "team_color", "team_color_secondary", | ||
| "active_start_year", "active_end_year", "team_stints" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use subset assertions for the expanded column list.
Adding these fields is valid, but expect_setequal() requires the API response to contain exactly this set. Upstream additions will make the test fail; use the required subset direction instead:
Proposed fix
- expect_setequal(colnames(x), cols)
- expect_setequal(colnames(y), cols)
- expect_setequal(colnames(w), cols)
+ expect_in(sort(cols), sort(colnames(x)))
+ expect_in(sort(cols), sort(colnames(y)))
+ expect_in(sort(cols), sort(colnames(w)))Also applies to: 26-28
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/testthat/test-cfbd_player_info.R` around lines 6 - 7, Update the
expanded column assertions in the player-info tests to verify that the expected
fields are a subset of the API response rather than requiring exact set
equality. Apply this change to the assertions around the listed columns and the
corresponding lines 26–28, preserving all required field names.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Table of Contents generated with DocToc
Pull Request
Summary
Deprecates the
yearargument inespn_cfb_team_coaches(), as the API only returns the coach for the most recent season regardless of year provided.Type of Change
feat: New feature (e.g. newcfbd_*,espn_cfb_*, orload_cfb_*wrapper)fix: Bug fixdocs: Documentation only (roxygen, README, NEWS, vignettes)test: Adding or updating tests (includes the pbp equivalence harness)refactor: Code refactoring (no functional change -- e.g. extracting helpers intoR/pbp_*.R)chore: Maintenance / tooling (.Rbuildignore,tools/, lockfiles)perf: Performance improvementci: GitHub Actions / workflow changesRelated Issues
Closes #125
Background & Context
espn_cfb_team_coaches()no longer provides a list of coaches for a team in a given season and now only returns the coach for the most recent season, even though it returns the year provided to the function. See issue #125 for more details.Changes Made
This endpoint still requires a season specification, so I've changed the year argument to default to
most_recent_cfb_season(). This argument now kicks a warning if a year other than the most recent season is provided, and the function returns the current year coach to avoid breaking any pipelines using this function.Submission Checklist
snake_case, 2-space indent, native pipe|>)tryCatchblock (e.g.df_list <- list(),plays_df <- NULL)dplyr::select(-dplyr::any_of(...)); renames usedplyr::rename(dplyr::any_of(c(new = "old")))cli::cli_alert_*()/cli::cli_warn()/cli::cli_abort()(notmessage()/stop()/warning())devtools::document()has been run (NAMESPACE updated; no hand-edits toman/orNAMESPACE)@export,@family,@return, and a runnable example (\donttest{}for live-network calls)tests/testthat/withskip_on_cran()+skip_on_ci()and a skip-if-empty guard right after the API callexpect_in(sort(expected), sort(colnames(x)))test-pbp_equivalence.Rstill passes (or the allow-list is updated with justification)devtools::check()passes with no errors or warningsNEWS.mdupdated under the current# **cfbfastR 2.3.0**heading (if user-facing)cran-comments.mdupdated (if behavioural / user-visible)_pkgdown.ymlupdated for new exports that need an explicit reference entry (thestarts_with("cfbd_")/starts_with("espn_cfb_")selectors pick up most prefix-matching additions automatically)README.Rmdre-rendered withdevtools::build_readme()if README content changedDESCRIPTIONnormalized withusethis::use_tidy_description()if it was editedNEWS.md/CLAUDE.md/CONTRIBUTING.md/.github/copilot-instructions.md/.github/pull_request_template.mdchangedtype: description); no AI co-authorsTesting
Describe how you verified these changes work correctly:
devtools::test()(offline -- network tests skip cleanly)devtools::test()withCFBD_API_KEYset (live CFBD)devtools::test()with ESPN connectivity (liveespn_cfb_*)tests/testthat/test-pbp_equivalence.R(if you touched PBP / EPA / WPA)Reviewer Checklist
Reviewers: Do not approve until all items are verified
NEWS.md,cran-comments.md,_pkgdown.yml, roxygen) has been updated or marked N/A with justificationAdditional Notes
Summary by Sourcery
Deprecate the year parameter in espn_cfb_team_coaches to align with ESPN’s current API behavior and update tests accordingly.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
Breaking Changes
Documentation
Tests