search

daattali / ggExtra

📊 Add marginal histograms to ggplot2, and more ggplot2 enhancements
http://daattali.com/shiny/ggExtra-ggMarginal-demo/
Other
383 stars 45 forks source link

Create global_vars.R #130

Closed IndrajeetPatil closed 6 years ago

IndrajeetPatil commented 6 years ago

To appease the R CMD CHECK

daattali commented 6 years ago

Thanks!

Have you done this entirely through the web browser interface? I assume so because it was done so quickly. That's a great simple way to do a PR, especially a first PR, and I recommend it for very trivial changes to documentation. But for anything that involves code or anything that can potentially alter behaviour, it's much better to take the longer route or forking, making the changes locally, testing everything, and submitting as a PR after verifying. I know it doesn't seem like it's worth going through all this trouble for such a simple change, but it's a very important habit to get used to, because something even the most trivial changes can have unexpected bugs. I only recommend using the web interface if fixing typos in a README file.

Another cool trick is that if you include the words "fixes 126" in the title of the PR, then the issue will automatically close when this PR is merged.

daattali commented 6 years ago

It actually looks like Travis caught an error in this PR: https://travis-ci.org/daattali/ggExtra/builds/418472633?utm_source=github_status&utm_medium=notification

Error in c(".", "..density..", ) : argument 3 is empty

Good example of why PRs should always be done locally and tested :)

daattali commented 6 years ago

Thanks!

IndrajeetPatil commented 6 years ago

Yeah, I kind of took a shortcut because this seemed like such a minor change. Plus, I knew Travis or Appveyor builds would catch if something was wrong. But, next time, I'll create a local repo and run all checks.