Add module: how to do PR reviews

[lindsayplatt]

Right now, we briefly touch on what to expect from someone who is reviewing your PR (see the 8th step in the course) BUT we never really go into how to do one yourself. This is a critical skill. It might be a lot to squeeze into this course, so maybe it is an additional module at the end. I could see it starting out like this:

You are almost ready to be a Git project collaborator! Being a collaborator on a Git project will require you to contribute code and receive peer review (as you have done already in this course), but it will also require you to provide peer review of others' code. Let's talk about how to perform a PR review for a collaborator's code.

Consider diving into the mechanics, as well as, the concepts (this list needs to be checked for completeness):

• High level check: skim all files. Are there any being checked in that shouldn't be? E.g. big data files, non-code?
• Look at style, documentation, and comments. Are there things that are difficult to follow?
• Test the code. Check the code out locally and actually verify that it works and does what it should. Sam Oliver suggested this handy post describing how to use usethis functions to aid in that review process if you are in R. This course is somewhat language agnostic, but might be worth mentioning this resource.

[jds485]

I'd say that your first 2 bullets are gathered from the provided supplementary reading (in onboarding) on how to do PR reviews. But if there's a commenting style you all like to use for reviewing these repo components, that would be great to add to this training! The testing of code is not covered as much in the readings, and I would be interested in hearing how you're doing this. I have been cloning the branch corresponding to the PR and running it locally. Is there a more efficient way?

[limnoliver]

I wonder if one of the monthly DS practitioners meeting could facilitate a deep-dive into this conversation? I'd be especially interested in how new folks have handled PR review outside of our organization. Hearing how others choose when to pull down the PR and run locally, what they're looking for in a PR, etc., might help us standardize some of these practices or make them more robust. We've also talked about adding some style/syntax tests, either through the PR review process or through continuous integration, with something like lintr (though this is R-specific).

[jesse-ross]

@limnoliver I think this would be great to cover in a monthly DS practitioners meeting; I'm making a note that we should cover it; thanks! Linting is definitely something that should help make PR review easier, as we ease into it (and we can use pylint to do it for python).

[jesse-ross]

@lindsayplatt Another thing that we might add to this is examining test coverage (for functions) or assertion coverage (for inputs). I don't yet have a good feel for how widespread these kinds of tests are in our working code, but to the extent that they are there, checking them would generally be part of reviewing the PR.

[limnoliver]

Personally, I feel like formal testing is one of my weakest code practices/biggest blind spots. Would propose this as another DS practitioners discussion topic!

Also adding I don't see a ton of formal tests in project code, either, so maybe I'm not alone?

[lindsayplatt]

Adding a comment here to resurface this discussion. Seems like this could be useful as you think more about documenting data science best practices @jesse-ross