search

cdalzell / Lahman

R Package Containing Sean Lahman's Baseball Database
https://cran.r-project.org/web/packages/Lahman/Lahman.pdf
77 stars 37 forks source link

Dm help page edits #22

Closed djmurphy420 closed 8 years ago

djmurphy420 commented 8 years ago

Update of the help pages to use dplyr, tidyr and ggplot2 over plyr and lattice. Check the commit comments. I modified Michael's batting champion average plot using only one offset column instead of two in the Batting help page - there is some clumping of names in one region. If Michael finds that unacceptable, I could try the ggrepel package to separate the labels or revert to a modified version of his offset labels (modified because the order of the players isn't the same in the dplyr version of the output data). In the Schools help page, I couldn't get the last (pre-existing) example to work because the zipcode object doesn't exist even after the package is installed and loaded. I tried geocoding schools as a substitute, but several of the geocodes were (wildly) inaccurate among the first 10 I tried (testing with revgeocode) so I gave up on the idea. The dplyr code to create the object 'zips' is purely an abstraction and untested (unlike the other code).

I would have used your dplyr-update branch, but it left me in a detached HEAD state and I lost my first set of commits. Instead, I created my own branch and pushed the commits to my forked version of the repo...this is why you're getting a pull request :)

Hope you like the changes! Dennis

friendly commented 8 years ago

Wow! That's a lot of work. I'm looking forward to reviewing how you did these things.

However, these commits should not be merged with/pulled into master (which still reflects v. 4), but rather into the develop (which is our current v. 5.0 branch). You can see in the previous commits list that there are many conflicts wrt master, and the fact that this cannot be merged without resolving conflicts.

I think Chris is unavailable for a bit to referee this, so for now can you please withdraw this pull request and re-do it with develop as the target. If you can't do this, I'll try to figure it out.

cdalzell commented 8 years ago

Wow! Agree with @friendly, there's a lot there and I'm looking forward to taking a gander at it!

Also agree with @friendly, master is not the right destination to do a PR against. Where that right place is (probably develop), that's a good question that I'll look into when I'm not attempting to tap out a response on a cell phone while wearing rubber gloves. I don't recommend it! :)

As for me things are going well but slowly. I'm expecting to be back in the mix Sunday.

djmurphy420 commented 8 years ago

I closed the pull request. Can I move the branch on my fork from master to develop and make a new pull request or do I need to create a new one under develop and redo all the commits? (Other alternatives??) My apologies for not being at all facile in Git, but I'm trying :) @Chris: Take your time. We can cover for you in the interim - you've got more important things to worry about. D. 

On Wednesday, August 17, 2016 7:34 PM, Michael Friendly <notifications@github.com> wrote:

Wow! That's a lot of work. I'm looking forward to reviewing how you did these things.However, these commits should not be merged with/pulled into master (which still reflects v. 4), but rather into the develop (which is our current v. 5.0 branch). You can see in the previous commits list that there are many conflicts wrt master, and the fact that this cannot be merged without resolving conflicts.I think Chris is unavailable for a bit to referee this, so for now can you please withdraw this pull request and re-do it with develop as the target. If you can't do this, I'll try to figure it out.— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.