-
Notifications
You must be signed in to change notification settings - Fork 19
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
32 fetch all players in league #65
32 fetch all players in league #65
Conversation
…ers in a league. players api is paginated so solution loops over all pages
|
||
|
||
class League: | ||
def __init__(self, ctx, league_id): | ||
self.ctx = ctx | ||
self.id = league_id | ||
self.players = list() |
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.
I couldn't find a usage for these players list. To make function naming standard (teams(), standings()), I removed these class attribute and added a method instead.
def players(self, persist_ttl=DEFAULT_TTL): | ||
logger.debug("Looking up players") | ||
START = 0 | ||
COUNT = 25 |
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.
these START and COUNT variables may come from a constants/config file. As there are no other usages, I choose to use them inside the method directly.
25 is the max limit on yahoo fantasy API. I tried to use this max value to decrease the number of requests. I'm not sure is there any benefit for the user to use another value on count.
logger.debug("Looking up players") | ||
START = 0 | ||
COUNT = 25 | ||
data = self.ctx._load_or_fetch(f"players.{self.id}.{START}", f"players;count={COUNT};start={START}", league=self.id) |
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.
I created a custom persistance path to handle players collection.
These load or fetch lines has more than 79 characters, I can format them to comply with PEP8.
from_response_object(p, player) | ||
players.append(p) | ||
START += COUNT | ||
data = self.ctx._load_or_fetch(f"players.{self.id}.{START}", f"players;count={COUNT};start={START}", league=self.id) |
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.
to make a better and faster improvement, these request may be sent simultaneously in the future.
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.
adds some comments to check
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.
Looks great, thanks for putting this together!
@mattdodge
I have implemented a solution for #32
Can you please check it and review? I will also adding some comments to PR. You can check these too.