rudeboybert / fivethirtyeight

R package of data and code behind the stories and interactives at FiveThirtyEight
https://fivethirtyeight-r.netlify.app/
Other
454 stars 104 forks source link

Adding nba raptor dataset #61

Closed danicamiguel closed 4 years ago

rudeboybert commented 4 years ago

Hi @danicamiguel, thanks for your PR. A couple of things right off the bat:

@beanumber

danicamiguel commented 4 years ago

Hi @rudeboybert I have added the files and updated my commit history. Let me know if it is ok.

beanumber commented 4 years ago

@danicamiguel note the following WARNING on Travis:

* checking for missing documentation entries ... WARNING

Undocumented code objects:

  ‘nba_raptor’

Undocumented data sets:

  ‘nba_raptor’

All user-level objects in a package should have documentation entries.

I don't think your roxygen tags are quite right. Pay careful attention to the name of each piece of documentation.

danicamiguel commented 4 years ago

Hi @rudeboybert I have made changes to your comments as listed:

    • [X] Could you put era first in the list of variables? That way it matches order of columns in the data frame?
    • [X] Just so we can stay consistent in coding style throughout this package, could you do this with a mutate? Ex: mutate( raptor_box_offense = NA, raptor_box_defense = NA, ... )
    • [X] Could you do the converting to factor after you've bind_rows() the data frame? Otherwise the saved levels are off. Doing it this way yields era being a character
    • [X] Could you do this with a select()? Again, so that we have consistency of tidyverse style for this package and not base R? Make sure to put the previous mutate() and this select() all in the same pipe %>% chain.
    • [X] All comments for historical_raptor_XXX apply here for modern_raptor_XXX as well
beanumber commented 4 years ago

@danicamiguel the conflicts noted above in DESCRIPTION and NEWS.md can most likely be resolved by syncing your fork. Can you try that?

rudeboybert commented 4 years ago

Hey @beanumber, it seems after approving @danicamiguel's changes, I was able to resolve the merge conflict myself. Given how often DESCRIPTION and NEWS.md is changing because of all the PR merging going on, I think that it was easier for me to do it.

@danicamiguel thanks for your PR! You're now listed as a package contributor: https://github.com/rudeboybert/fivethirtyeight/graphs/contributors

rudeboybert commented 4 years ago

Also heads up @danicamiguel, I did a little more cleaning of your code to match the tidyverse style guide.

Strictly a style thing, nothing functional changed in your code

beanumber commented 4 years ago

Awesome! Thanks @rudeboybert !