Added get_roster function that takes in team abbreviation and year to…#276
Added get_roster function that takes in team abbreviation and year to…#276PGatts wants to merge 2 commits intojaebradley:v4from
Conversation
… return roster of team from that year
jaebradley
left a comment
jaebradley
left a comment
There was a problem hiding this comment.
@PGatts sorry for the delayed review!
This is a cool feature to add - hopefully my feedback makes sense!
| @@ -1,4 +1,6 @@ | |||
| #!/Users/jaebradley/projects/basketball_reference_web_scraper/bin/python3 | |||
| ) | ||
| return output_service.output(data=values, options=options) | ||
|
|
||
| def get_roster(team, year=None, output_type=None, output_file_path=None, output_write_option=None, json_options=None): |
There was a problem hiding this comment.
Let's make year a non-optional argument. I don't think there's a pattern for any year values being optional (intentionally).
| ) | ||
| return output_service.output(data=values, options=options) | ||
|
|
||
| def get_roster(team, year=None, output_type=None, output_file_path=None, output_write_option=None, json_options=None): |
There was a problem hiding this comment.
Also, can we rename year to season_end_year? Following the naming convention in similar season-related methods.
| values=http_service.get_team_roster(team=team, year=year) | ||
| except requests.exceptions.HTTPError as http_error: | ||
| if http_error.response.status_code == requests.codes.not_found: | ||
| raise InvalidTeam(team=team, year=year) |
There was a problem hiding this comment.
I think this error's name is slightly inaccurate - the team value could be valid, but the season end year value could be invalid, like https://www.basketball-reference.com/teams/BOS/1020.html
I'd prefer to call this error InvalidTeamSeason (or something similar).
(Note that I've made similar inaccurate naming mistakes in other methods, like InvalidDate, that need to be corrected in the future.)
| file_options=FileOptions.of(path=output_file_path, mode=output_write_option), | ||
| output_type=output_type, | ||
| json_options=json_options, | ||
| csv_options={"column_names": "Players"} |
There was a problem hiding this comment.
Here's the Roster table
Happy to add more values in the future, but at a minimum the values should be
nameslug(orplayer_id)
| game_links = self.html.xpath(self.game_url_paths_query) | ||
| return [game_link.attrib['href'] for game_link in game_links] | ||
|
|
||
| class TeamRoster: |
There was a problem hiding this comment.
Can we create a SeasonTeamPage class that has a property called @team_roster_table?
Most of all the client methods refer to a top-level Page class that might expose underlying tables with properties or methods.
| @property | ||
| def roster_query(self): | ||
| return '//table[@id="roster"]//td[@data-stat="player"]' | ||
| @property |
There was a problem hiding this comment.
Nit: let's have a new line between lines 879 and 880.
| def roster_query(self): | ||
| return '//table[@id="roster"]//td[@data-stat="player"]' | ||
| @property | ||
| def team_roster(self): |
There was a problem hiding this comment.
Let's create Row classes to represent the underlying row content (to keep consistent with similar patterns in this file).
|
The related issue is #271 |
…nction. Also made season_end_year non-optional
2 participants
… return roster of team from that year