NBA: adding TypeError checks around operations #394
Conversation
SeanAmmirati
changed the title
There was a problem hiding this comment.
Thank you for the PR @SeanAmmirati, this is very nicely done (side note, I love the profile picture)! I think this is a wonderful update and it's always nice to cleanly handle errors.
I think the code looks great, and am happy to merge, but if you don't mind, I think it would be good to include some test cases which will run into these situations just to verify in the testing suite that they are functioning as expected. This could probably sit in the test_nba_boxscore unit tests at the end. Let me know if you have any questions and I will be happy to help out.
On a related note, the action used to upload coverage reports to Codecov is currently broken for updates from forked repositories, but I have a PR (#397) which should resolve this issue. I will merge that once its tests complete so hopefully coverage reports on future updates (like this) are able to be displayed properly, and all actions can complete as expected.
Lastly, I would be very happy to accept similar PRs for other sports too if you desire down the road! I'm always happy to work with the community to make this package more robust!
Thanks again for this wonderful update!
| try: | ||
| result = float(self.away_two_point_field_goals) / \ | ||
| float(self.away_two_point_field_goal_attempts) | ||
| except TypeError: | ||
| result = None | ||
| else: | ||
| result = round(float(result), 3) | ||
| return result |
SeanAmmirati
force-pushed
the
operator_bugfix
branch
from
73175c2
to
3b17831
Compare
|
Hi @roclark, thanks for the quick response! Happy to help in any way I can. Thanks for creating this package -- it is incredibly useful and helpful. More than happy to add the test cases, will do that shortly and check back once this has been done. Also apologies, I force pushed another small edit to avoid creating another commit, just fyi in case there are any conflicts in the history. Thanks! |
|
Just added some unit tests. Please let me know if this formatting works or if you have any suggestions. Also, thanks for the note on the profile picture, GLaDOS! The cake is a lie! 👍 Thanks! Sean |
SeanAmmirati
force-pushed
the
operator_bugfix
branch
from
d705aca
to
686f20c
Compare
…nto operator_bugfix
Codecov Report
@@ Coverage Diff @@
## master #394 +/- ##
=======================================
Coverage 99.97% 99.97%
=======================================
Files 48 48
Lines 10590 10616 +26
=======================================
+ Hits 10587 10613 +26
Misses 3 3
Continue to review full report at Codecov.
|
|
Hey @SeanAmmirati, sorry for not getting back to you earlier - it's been a very long week. These updates look great, and I appreciate the work on the test cases! Also, I'm happy to see the the codecov reports are now working on forked updates. This PR looks in great shape to me, so I will go ahead and merge it with master. Thank you again both for the update and for your patience! This is absolutely wonderful! |
This small PR simply adds some exception handling to some of the calculations for the NBA boxscore classes.
I also intend to do this for the other classes as well.
This was done because in the current state, running something like the following would raise a
TypeError:This was because the information that would have been required for all of these operations were valued None. While the decorators handle this issue for the particular values, the
TypeErrorwould happen with the arithmetic operators.This assumes that the attributes will only be of the correct type (float or int) or NoneType.
This should also be done for the other classes. It appears that this will be a problem for very old games. For reference, this was the first NBA basketball game available on basketball reference.
Happy to discuss this further or perhaps consider other ways to handle this. Also will be happy to do this for the other classes as I get to them.