search

spacetelescope / style-guides

An opinionated guide on how we work.
Creative Commons Attribution 4.0 International
55 stars 33 forks source link

PR review guide (aka "Code Review") #74

Closed brechmos-stsci closed 5 years ago

brechmos-stsci commented 5 years ago

There were questions that came up as part of one sprint group I am with about code reviews / Github PR reviews. So, I decided to write up a semi-opinionated piece on what should likely be part of a "code review". There are parts of this in which the details are part of other documents and I did not want to replicate here (e.g., Python code style).

Fixes #48

brechmos-stsci commented 5 years ago

This might fix https://github.com/spacetelescope/style-guides/issues/48 but at least contributes some answers to that issue.

pllim commented 5 years ago

Wow, I am about to review a PR about code review.

RuntimeError: maximum recursion depth exceeded
pllim commented 5 years ago

Some things mentioned here, like the change log check, could be automated (@drdavella knows how!).

brechmos-stsci commented 5 years ago

@pllim I addressed most of your ideas (I might have hit resolve conversation a little judiciously :) ). A few of the "should it be in here" I think we should leave. This is a broad guide and people can take what they need from this for their specific project. Also, too, some people may not have an idea of even where to start, so leaving more in than less, I think is beneficial.

stscicrawford commented 5 years ago

Looks okay to merge after final comments by @eteq

brechmos-stsci commented 5 years ago

@eteq I made all the changes requested except the last one. Feel free to push back on it if you want, but I think "Members" is more general and all-encompassing compared to "Engineers".

eteq commented 5 years ago

LGTM, so will go ahead and merge. Thanks @brechmos-stsci !