-
Notifications
You must be signed in to change notification settings - Fork 677
feat: added GravityLeaders Endpoint #626
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
base: master
Are you sure you want to change the base?
Conversation
|
@swar Here's an updated PR for only the |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
brandonhawi
left a comment
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.
Left a few comments, but awesome work so far.
Perhaps a more philosophical question from a library standpoint, but with this type of data perhaps we'd want to validate at the library level to ensure that users don't query for this data for unsupported past seasons? I'm sure the NBA API itself would error but perhaps the library's error will be more useful?
| } | ||
|
|
||
| # Initialize dataset attributes | ||
| self.league_leaders = None |
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.
| self.league_leaders = None | |
| self.leaders = None |
For consistency with the nba_api, might be better to just use their nomenclature. What do you think?
Also on a side note, why init to None here? It will always be set in load_response.
| ) | ||
|
|
||
|
|
||
| class GravityLeaders(Endpoint): |
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.
Should probably provide this class with a docstring?
| LEADER_FIELDS = ( | ||
| "PLAYERID", | ||
| "FIRSTNAME", | ||
| "LASTNAME", | ||
| "TEAMID", | ||
| "TEAMABBREVIATION", | ||
| "TEAMNAME", | ||
| "TEAMCITY", | ||
| "FRAMES", | ||
| "GRAVITYSCORE", | ||
| "AVGGRAVITYSCORE", | ||
| "ONBALLPERIMETERFRAMES", | ||
| "ONBALLPERIMETERGRAVITYSCORE", | ||
| "AVGONBALLPERIMETERGRAVITYSCORE", | ||
| "OFFBALLPERIMETERFRAMES", | ||
| "OFFBALLPERIMETERGRAVITYSCORE", | ||
| "AVGOFFBALLPERIMETERGRAVITYSCORE", | ||
| "ONBALLINTERIORFRAMES", | ||
| "ONBALLINTERIORGRAVITYSCORE", | ||
| "AVGONBALLINTERIORGRAVITYSCORE", | ||
| "OFFBALLINTERIORFRAMES", | ||
| "OFFBALLINTERIORGRAVITYSCORE", | ||
| "AVGOFFBALLINTERIORGRAVITYSCORE", | ||
| "GAMESPLAYED", | ||
| "MINUTES", | ||
| "PTS", | ||
| "REB", | ||
| "AST", | ||
| ) |
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.
While this is consistent with the rest of the codebase, can we consider having a single source of truth for the field names instead of two places (here and in the GravityLeaders class)?
|
Hey @brandonhawi please re-review when you get a moment. |
Added the new Gravity Leaders endpoint. The NBA defines Player Gravity as "Gravity quantifies how much a player pulls defenders towards them above expected, essentially measuring how much attention they draw compared to what the spacing on the floor predicts."