Skip to content
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

Fix London start_times by using meridiem from nflgame.sched #202

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

weixiyen
Copy link

Currently the schedule inserter for the DB doesn't account for London games correctly.

NFLGame now has meridiem info for all 2016 games and beyond, so NFLDB should take advantage of it for accurate game start_time information.

If there is no meridiem, we default to "PM".

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@BurntSushi
Copy link
Owner

Thanks for doing this! I really wish we had better test coverage, because I don't know how to tell if this actually works without pulling it down and trying it, which means it's going to take longer to merge. Manual QA sucks.

I see you removed some special cases. Is that right? Does meridiem info exist for those games now too? (Which occurred before 2016.)

@maa42
Copy link

maa42 commented Oct 20, 2016

Wow. Fixed my London issue right before the Giants game this Sunday. Thanks for the help

@weixiyen
Copy link
Author

weixiyen commented Oct 23, 2016

So the XML does not contain any of that info.

Meridiem is calculated in NFLGame only, based on guessing, but it can return AM, PM, or None.

https://github.com/BurntSushi/nflgame/blob/17f2f866302383adaef6c73b00483ddf64263e6c/nflgame/update_sched.py#L72

When it is None, I just default to PM.

I'll have to check how well this works, but at least it works for every game in 2016, it might even be true for older games... if they are played around same time of day, as meridiem is calculated based on time of day.

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.

None yet

3 participants