nfl.schedule.Game.result return win/loss/tie/unknown correctly

[DavidLiuGit]

1. Add a constant TIE to represent NFL games that can end in a tie, at the end of overtime.
2. in schedule.Game.result, consider all possible outcomes of an NFL game, instead of returning WIN indiscriminately if the if-statement does not evaluate to true. Check whether the result is l/w/t and return the corresponding constant. If none of the above match, return None

closes #245

[roclark]

This looks good to me! This is a great addition, and is very nicely implemented. One item I noticed is that the checks I created weren't run on this PR. This should be something on my end that I need to fix as I believe they trigger only on a branch push to the repository, but not on a PR create. Since this branch is in your personal fork, the checks weren't triggered as the repo only sees a PR creation. This is something I need to fix on my end and I would like to merge it prior to this update so the checks are properly run. Is that fair?

Also, one thing we might run into with the checks is a drop in testing coverage. I don't believe my current testing suite covers ties or games that haven't been played yet, so it would be good to test those scenarios too. Would you be able to add some unit tests to cover these scenarios? Let me know if you would like any help.

Thanks again for creating this! Once I fix the checks and the other items I mentioned get knocked out, I will be happy to merge this! 😃

[roclark]

This is a bit of a nit-pick, but would it be possible to wrap this text with the previous line? So, something like:

"""
Returns a ``string`` constant …
the game. NFL games may end in a …
"""

I've had issues in the past with the auto-documentation with a singular newline like this, and found it's easiest to just wrap together for another sentence. Let me know if you have any issues with this though! Also, looks like this sentence could use a period after the end after None.

[DavidLiuGit]

Ah, didn't know about the auto-doc issue. I will update this

[DavidLiuGit]

I'll have a look at the unit tests and add the new cases this weekend. Although I foresee a potential issue writing tests for unplayed games, since those games will eventually get played. Do you have any advice?

[roclark]

Hey @DavidLiuGit, great point! I ran into this issue when I first devised the testing suite, also knowing that certain aspects like career stats will change as well. To get around this, I replace a call to a live page on sports-reference.com with a call to read a copy of a HTML page stored in the tests directory for each component. That way, regardless of what day it is, or what games have been played, the tests will always be run against the same data. Of course, there are some flaws with this as it doesn't always catch any changes that happen on the website, but I've been working on a secondary testing suite which runs tests against live data. I won't be able to test the actual values against known results with this secondary suite, but it will at least let me know if the algorithms will successfully run.

On a more relevant note for this update, these tests would likely live in the TestNFLSchedule unit test module. You can find an example for the wins and losses here. I would recommend copying the template of these two functions, and add one function for a tie, and one for if the result is empty.

Again, let me know if you have any questions or concerns with this and I will be happy to help! With your other PR merged, the testing suite should now run automatically on an update.

Thanks again!

[DavidLiuGit]

I added unit tests for tied games and games with no results, and updated the function description. Let me know if there's any other changes needed!

[roclark]

Hey @DavidLiuGit, this looks good to me! The tests are passing and the new code you added is being covered by the unit tests you created, so I'm happy with this! Let me know if you are planning on any further updates, otherwise I will accept this!

[DavidLiuGit]

I'm all done with this