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.

Sign up for GitHub

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

Jump to bottom

Update NFL season URL #250

Merged
merged 1 commit into from
Nov 14, 2019
Merged

Update NFL season URL #250

merged 1 commit into from
Nov 14, 2019

Conversation

DavidLiuGit
Copy link
Contributor

PFF's season page moved to a new URL. Updating SEASON_PAGE_URL in nfl.constants to reflect the change.

@DavidLiuGit DavidLiuGit mentioned this pull request Nov 12, 2019
Closed
@roclark roclark self-requested a review November 13, 2019 02:00
@roclark roclark added the bug Something isn't working label Nov 13, 2019
@roclark roclark added this to the Release 0.4.7 milestone Nov 13, 2019
roclark
roclark reviewed Nov 13, 2019
View reviewed changes
sportsreference/nfl/constants.py Outdated
@@ -352,7 +352,8 @@
'punt_return_touchdowns': 'td[data-stat="punt_ret_td"]'
}

SEASON_PAGE_URL = 'http://www.pro-football-reference.com/years/%s.html'
# SEASON_PAGE_URL = 'http://www.pro-football-reference.com/years/%s.html'
Copy link
Owner

Choose a reason for hiding this comment

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

Since this URL is no longer valid, we should probably get rid of this line entirely. I know it won't be used as it's a comment, but I think it would look cleaner if we just took it out altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed!

roclark
roclark reviewed Nov 13, 2019
View reviewed changes
Copy link
Owner

@roclark roclark left a comment

Choose a reason for hiding this comment

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

Looks good to me! I just have my one note about removing the comment and this should be good! It looks like this should clear up the unit tests too which is great! Thanks for putting this together!

@roclark
Copy link
Owner

roclark commented Nov 13, 2019

On further inspection, it looks like this PR should completely resolve the issues with the push tests, so I would like to get this merged prior to any other code so the checks on other updates will be valid.

@roclark
Copy link
Owner

roclark commented Nov 14, 2019

Thanks for the update @DavidLiuGit! This looks great! It seems that some of the tests are failing here, but they don't appear to be related to your change, so I would like to move forward with merging this and work on other updates moving forward. Thanks again for taking care of this and for the contribution!

@roclark roclark merged commit 6849c91 into roclark:master Nov 14, 2019
@DavidLiuGit DavidLiuGit mentioned this pull request Nov 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roclark roclark roclark left review comments

Assignees

@DavidLiuGit DavidLiuGit

Labels
bug Something isn't working
Projects
None yet
Milestone
Release 0.4.7
Development

Successfully merging this pull request may close these issues.
2 participants
@DavidLiuGit @roclark