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

Feedback andres #60

Merged
merged 3 commits into from
Mar 23, 2020
Merged

Feedback andres #60

merged 3 commits into from
Mar 23, 2020

Conversation

AndresPitta
Copy link
Collaborator

Hi guys,

This commit contains the following changes:

  • The README.md file was modified to contain information about the dependecies and how to install the correct version of them. This addresses @kvarada's following feedback on issue #31:

here

Installation directions might benefit from some clarification. I had to do the following in order to get it working.

conda install selenium
pip install python-semantic-release

here

Good that you include the instruction to put the chromedriver path in the system path variable. I guess you do not mean PATH_TO_CHROMEDRIVER.EXE in the following command for Linux and MacOS.

export PATH="<PATH_TO_CHROMEDRIVER.EXE>:$PATH"

and here

Tests
When I ran your tests, two tests failed with the following error.
AttributeError: module 'altair.vegalite' has no attribute 'v4'

  • The README.md file was also modified to have the correct example for the function nba_ranking, in order to address @kvarada and @zoepan00 feedback on issue #31:

here:

Step 3 example nba_ranking(nba_2018_regular, 'PLAYER' , 'GP', top = 2, ascending = False, fun = 'mean') is not working

and here:

In your README, in the following usage example of the function nba_ranking, the 'PLAYER' column is undefined in the scraped CSV and so it gives an error. Replacing it with 'NAME' worked for me.
nba_ranking(nba_2018_regular, 'PLAYER' , 'GP', top = 2, ascending = False, fun = 'mean')

  • nba_ranking's error message was updated to address @zoepan00's feedback on issue #31:

nba_ranking: Error message for fun argument is not updated. Currently it says The fun argument should be either var or mean

Please check everythin is in order,
Thanks,
Andres

@codecov
Copy link

codecov bot commented Mar 23, 2020

Codecov Report

Merging #60 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #60   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          295       295           
=========================================
  Hits           295       295           
Flag Coverage Δ
#unittests 100.00% <100.00%> (ø)
Impacted Files Coverage Δ
pysketball/nba_ranking.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb6e568...845954c. Read the comment docs.

@vanandsh vanandsh assigned vanandsh and unassigned vanandsh Mar 23, 2020
Copy link
Collaborator

@vanandsh vanandsh left a comment

Choose a reason for hiding this comment

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

All changes look fine! Thanks @AndresPitta

@vanandsh vanandsh merged commit 0322978 into master Mar 23, 2020
@AndresPitta AndresPitta mentioned this pull request Mar 25, 2020
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.

2 participants