Add Edge Endpoints#113
Conversation
|
This is great, thanks! I'll check this over shortly. As for where to put the edge stats, I was thinking about this recently and So |
|
Yeah those were my thoughts as well for making the edge stats their own class. Let me know if you need any changes from me! |
|
I will try to hit this tonight after work, I am actively working on some ML stuff so this is going to be super helpful. First glance it looks good. @mattanikiej |
coreyjs
left a comment
coreyjs
left a comment
There was a problem hiding this comment.
Just a few minor things I noticed, to make it match up with the other components. Otherwise it looks good, I like how each method is named appropriately. Thanks for putting in the work on this one.
| # SKATER ENDPOINTS | ||
| # ======================== | ||
|
|
||
| def skater_detail(self, player_id: int, season: int = None, game_type: int = None) -> Dict[str, Any]: |
There was a problem hiding this comment.
| def skater_detail(self, player_id: int, season: int = None, game_type: int = None) -> Dict[str, Any]: | |
| def skater_detail(self, player_id: str, season: str = None, game_type: int = None) -> Dict[str, Any]: |
Let's use strings for player_id and season for this and the other methods, I found it makes using the library a bit easier. This would match things like stats.player_game_log player_game_log(self, player_id: str, season_id: str, game_type: int)
There was a problem hiding this comment.
Then update the doc string also to match.
There was a problem hiding this comment.
For season, do you want me to change it to season_id? The wiki with the edge endpoints you made uses season, but I just noticed the rest of your code uses season_id. Which one would you like me to use?
| resource = f"edge/team-landing/{season}/{game_type}" | ||
| return self.client.get(endpoint=Endpoint.API_WEB_V1, resource=resource).json() | ||
|
|
||
| def team_shot_speed_detail(self, team_id: int, season: int = None, game_type: int = None) -> Dict[str, Any]: |
There was a problem hiding this comment.
| def team_shot_speed_detail(self, team_id: int, season: int = None, game_type: int = None) -> Dict[str, Any]: | |
| def team_shot_speed_detail(self, team_id: str, season: str = None, game_type: int = None) -> Dict[str, Any]: |
Similar to player_id I think we should also use strings here.
|
One thing that is confusing me, related to the edge APIs is their use of This isn't directly related to this PR, but more about how confusing their endpoints are. |
|
Right on man, I'll take care of these after work tonight |
coreyjs
left a comment
coreyjs
left a comment
There was a problem hiding this comment.
One minor thing, then good to go.
| # SKATER ENDPOINTS | ||
| # ======================== | ||
|
|
||
| def skater_detail(self, player_id: str, season: str = None, game_type: int = None) -> Dict[str, Any]: |
There was a problem hiding this comment.
| def skater_detail(self, player_id: str, season: str = None, game_type: int = None) -> Dict[str, Any]: | |
| def skater_detail(self, player_id: str, game_type: int = 2, season: str = None) -> Dict[str, Any]: |
One more minor request I noticed the more I played around with it. Lets default game_type=2 (regular season), the reason being then when you do the check for
if season is None or game_type is None:
...it can instead be
if season is None:This alleviates some confusion where you would call
client.edge.skater_detail(player_id=1, season="20202021")
which would trigger the conditional and then return the /now version (2025-2026) instead of 2020-2021.
Lets do this for the rest of the endpoints, then its good to go. Ill aim for release in ver 3.1.0
There was a problem hiding this comment.
Sounds good, just pushed the changes. Let me know if you need anything else for these endpoints.
|
closes #112 , set for release version |
2 participants
Hey I noticed that in issue #112 you wanted to add the additional Edge stats endpoints that were listed in the wiki page here that you linked in the issue as well: https://github.com/coreyjs/nhl-api-py/wiki/NHL-API-Endpoints-2025-2026.
I had some free time so I thought I'd comb through them. I got most of them in there, but the top 10 ones such as
I couldn't get to work.
Also, as for the style, I wasn't sure if you intended to create a new Edge class, which is what I did, or if you wanted these wrappers in the Stats class, so I just made them a new class. The wrappers can be very easily ported over so it's no big deal.
I know there's probably much to change here, and I wasn't part of a discussion in the issue, but I'd love to contribute, and I'll be glad to make any changes.