Skip to content

Conversation

@kodarfour
Copy link

…d Updated documentation to reflect such changes (README and CONTRIBUTORS)

…d Updated documentation to reflect such changes (README and CONTRIBUTORS)
@kodarfour
Copy link
Author

apologies if this project doesn't want PR asking to merge. new programmer and just wanted to help.

…umentation to maintain consistency with Sleeper API
Copy link
Collaborator

@wfordh wfordh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRs are good! Left some comments, mostly bigger picture things about the direction of the package. Didn't know the API even supported the other sports, so thank you for bringing this up!

The league.py file LGTM.


def get_all_players(self):
return self._call("https://api.sleeper.app/v1/players/nfl")
def get_all_players(self, sport):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably have sport="nfl" as the default argument since this would change functionality for people and I assume most users are using the package for NFL data and not NBA or LCS.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtsong meant to tag you here. Any thoughts on adding support for these other sports?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the Sleeper API has those support, enabling that in the wrapper would be a good idea since the scope of this wrapper is primarily around the Sleeper API itself.

def get_all_players(self, sport):
if sport == "nfl":
return self._call("https://api.sleeper.app/v1/players/nfl")
if sport == "nba":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this should be elif sport == "nba" and then there should be an else statement with some sort of logging to let the user know if they provided a sport argument that is not supported by the API. I did double check and this call does return NBA players. lcs is also supported, so if we're gonna add one, then we should probably add both.

### Players.get_all_players(sport)
Gets all of the players in fantasy football or basketaball (Recommended to use once per day). Data returned looks like: https://docs.sleeper.app/#fetch-all-players

- sport: (str) The sport to get. Only "nfl" or "nba" right now.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming we add it, need lcs here to match line 342

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.

3 participants