Add Code Review guide #39

Closed
wants to merge 16 commits into
from

Conversation

Projects
None yet
10 participants
Contributor

croaky commented Jan 6, 2013

Asychronous code review via Github pull requests can be incredibly
efficient, but easy to accidentally offend or miscommunicate if we
aren't careful about our language choices or have a common expectation
of how to act when reviewing and responding to reviews.

This new guide attempts to set some basic expectations to streamline
code reviews and help us stay happy, healthy, and working closely as a
team.

croaky added some commits Jan 6, 2013

Add Code Review guide
Asychronous code review via Github pull requests can be incredibly
efficient, but easy to accidentally offend or miscommunicate if we
aren't careful about our language choices or have a common expectation
of how to act when reviewing and responding to reviews.

This new guide attempts to set some basic expectations to streamline
code reviews and help us stay happy, healthy, and working closely as a
team.
Highlight Friday technique discussions
Distinguish between discussions that are appropriate for code reviews
versus those that are appropriate for technique discussions.
code-review/README.md
+* Don't use sarcasm.
+* Reach resolutions quickly.
+* Talk in person if there are too many "I didn't understand" comments.
+* Talk in person if there are too many "Alternative solution:" comments.
@mike-burns

mike-burns Jan 6, 2013

Owner

For both of these, it'd be useful for the others involved if someone posts a follow-up summary, when possible.

@croaky

croaky Jan 6, 2013

Contributor

I like it. Added in a70d54a.

code-review/README.md
+* Talk in person if there are too many "I didn't understand" comments.
+* Talk in person if there are too many "Alternative solution:" comments.
+* Use emoji.
+* Use humor.
@mike-burns

mike-burns Jan 6, 2013

Owner

Both of these seem very optional.

@croaky

croaky Jan 6, 2013

Contributor

True. I didn't intend to make those seem required. What do you think of this edit? 6e44c8c

@mike-burns

mike-burns Jan 6, 2013

Owner

👍 🌈 🐝

(Looks good.)

code-review/README.md
+ this class/file/method/variable to make those reasons more clear?")
+* Extract some changes and refactorings into future tickets/Trello cards.
+* Link to the code review from the ticket/Trello card. ("Ready for review:
+ http://github.com/organization/project/pull/1")
@mike-burns

mike-burns Jan 6, 2013

Owner

Since we change story/ticketing systems every so often, how about saying "story card" instead of mentioning Trello?

@croaky

croaky Jan 6, 2013

Contributor

Yeah, I debated that. Was trying to use Trello as an example so it didn't seem too abstract or cause confusion between the pull request/Github system compared to a project management system. Changed it to "ticket/story" in 2f35000. Better?

@mike-burns

mike-burns Jan 6, 2013

Owner

That looks better to be, but I definitely understand your concern. Perhaps others will have some input on this?

code-review/README.md
+ > Order resourceful routes alphabetically by name.
+
+The style guidelines are not optional. They have been agreed to as a team,
+intended to improve readability, consistency, and save time discussing style.
@mike-burns

mike-burns Jan 6, 2013

Owner

I thought we had agreed that the style guidelines are actually somewhat optional; specifically, they should be broken only when careful consideration and judgement has been put into the thought.

@croaky

croaky Jan 6, 2013

Contributor

You're right. There's a few good lines in the style-guide/README.md that already cover this. I removed a few sentences in this section in 2d4d1a0.

code-review/README.md
+
+If you disagree with a style guideline, the code review is not the place to
+discuss it. Open an issue on the style guide repo. In the meantime, apply the
+guideline to the codebase.
@mike-burns

mike-burns Jan 6, 2013

Owner

This phrasing comes across as heavy-handed. Perhaps a lighter tone could work, like:

As with the rest of the guides, disagreements over the style guide are done via pull request, and such refactorings should not hold up the commit.
@croaky

croaky Jan 6, 2013

Contributor

Agreed. While editing in 2d4d1a0, I noticed the same tone and made a change. Better?

@mike-burns

mike-burns Jan 6, 2013

Owner

Definitely, though now the grammar seems a little off. I think changing discuss to discussing will fix it.

Owner

mike-burns commented Jan 6, 2013

If ever I was worried about being offensive on a pull request, this one is it!

croaky added some commits Jan 6, 2013

Remove commentary in style comments section
It gave conflicting advice to the "Don't violate the conventions without
a good reason." text within the style guide.
code-review/README.md
+ clarification. ("It's like that because of these reasons. How about I rename
+ this class/file/method/variable to make those reasons more clear?")
+* Extract some changes and refactorings into future tickets/stories.
+* Link to the code review from the ticket/story. ("Ready for review:
@drapergeek

drapergeek Jan 6, 2013

Owner

Is is necessary to give an example of how to add a link?

@croaky

croaky Jan 6, 2013

Contributor

Haha, maybe not.

What I was trying to communication was part of the complete documentation chain:

  1. git blame
  2. see ticket URL in commit message
  3. open ticket URL if desired
  4. see pull request URL on ticket
  5. open pull request URL if desired.

Was trying to be super-clear about which URL to use.

Okay to leave it?

@drapergeek

drapergeek Jan 6, 2013

Owner

Makes sense, I'm good with it.

protocol/README.md
Present-tense summary under 50 characters
* More information about commit (under 72 characters).
* More information about commit (under 72 characters).
+In the commit message, include the purpose of the change and link to any
@drapergeek

drapergeek Jan 6, 2013

Owner

Can we add an "If necessary" to this?

I've been seeing more documentation than necessary on certain projects and I think always linking back to the ticket is unnecessary.

@croaky

croaky Jan 6, 2013

Contributor

I think I combined two things in my first version which should be separated: linking to the ticket and linking to documentation. I agree with you that the documentation should be optional. I think the project management ticket should be linked if the author remembers (if the author forgets, no big deal, but creates a nice documentation chain if it can become a team habit).

Tried simplifying in 84f6956. Better?

@drapergeek

drapergeek Jan 6, 2013

Owner

We've been trying this process lately on my current project and I'm personally not a fan of it. There are a lot of times it's relevant and nice to have but I think putting in a guideline for it tends to make people see this as a rule and for me personally, I really don't want this to become anything like a requirement. I've seen a few things come out of this that I'm not a fan of.

  • Any commit with a story URL is questioned. There are some times you don't have a story associated with work, it's just a cleanup or refactoring.
  • Having the story URL attached gives the person submitting the story the ability to say "Look at the story" instead of actually explaining what the change does. I'm even less of a fan of that. The person who worked the story has the best knowledge of that story and should be available to explain it.
  • I've yet to actually have an instance of this being helpful.

I realize that part of my objections are based solely on my experiences with this current project but I have nothing else to go off of. Unfortunately once things are put in the guide they become canon and this is something I really don't want to support currently.

One nice benefit of this that I do like is that on pull requests that may need a bit of information, having the URL from the commit message bubble up into the description is quite nice. I would not be opposed to "recommending" that when applicable we put the story URL in description, but still not something I want to see become a requirement.

All that being said, if others think this is a great idea I will happily follow the rules 😄, just hasn't been something I've enjoyed at all on this project. It feels too much like bookkeeping for the same of bookkeeping, almost equivalent to having to track what you do every hour, just unnecessary.

Sorry for the long response but I wanted to make sure I expressed all my issues.

@croaky

croaky Jan 7, 2013

Contributor

@drapergeek I've pulled this line out of this pull request and submitted as a separate pull to discuss there:

#40

Owner

drapergeek commented Jan 6, 2013

I really like this idea. Is it possible to add something like:

Remember that when expressing yourself online, people don't always understand your intention so be more explicit and be overly nice.

Probably a better way to word that but I think a friendly reminder is always nice.

Seriously, love this idea.

croaky added some commits Jan 6, 2013

Remove "supporting documentation" line
Show, not tell, how to link to project management system ticket.
Contributor

croaky commented Jan 6, 2013

Remember that when expressing yourself online, people don't always understand your intention so be more explicit and be overly nice.

Yeah, that makes sense. Added that sentiment in 91c9a99.

code-review/README.md
+Code Review
+===========
+
+A guide for reviewing code with a light-hearted and respect attitude.
@adarsh

adarsh Jan 6, 2013

Contributor

Typo: respect attitude > respectful attitude

@croaky

croaky Jan 6, 2013

Contributor

Thanks, fixed: f977186

code-review/README.md
+* Talk in person if there are too many "I didn't understand" or "Alternative
+ solution:" comments. Post a follow-up comment summarizing offline discussion.
+
+Author of code for review
@adarsh

adarsh Jan 6, 2013

Contributor

Style: How about Author of code *under* review? Use of for sounds funky.

@croaky

croaky Jan 6, 2013

Contributor

Agreed. Changed: f977186

code-review/README.md
+
+If you disagree with a style guideline, open an issue on the style guide repo
+rather than discussing it within the code review. In the meantime, apply the
+guideline to the codebase.
@adarsh

adarsh Jan 6, 2013

Contributor

Would it be useful to indicate that adherence to Style is different than the Protocol or Best Practices guides?

I've started interpreting these as "Style should be adhered to, Protocol/Best Practices are suggestions". Unclear if this is the general perspective .

@croaky

croaky Jan 7, 2013

Contributor

Would it be useful to indicate that adherence to Style is different than the Protocol or Best Practices guides?

I don't think that's necessarily what we want. I think the high-level guidelines we have on the style guide can apply to all the guides. Just made that change in 5273cc8. Let me know what you think.

I've started interpreting these as "Style should be adhered to, Protocol/Best Practices are suggestions". Unclear if this is the general perspective.

I think they're all guidelines. We can save ourselves some low-level decision-making by following them. As the high-level guidelines say, "Don't violate a guideline without a good reason."

One issue that might cause confusion is that some of the best practice guidelines can be at odds with each other:

Prefer small objects with a single, well-defined responsibility.
Tell, don't ask.

Maybe we remove them from the guides and let Ruby Science deal with their nuances?

@adarsh

adarsh Jan 7, 2013

Contributor

I think your approach makes the most sense, to treat them all with the same level of gravity.

I'd like not to be so prescriptive about when to use them and not - they are called "guidelines" and that should be sufficient.

Good stuff. This is a good PR.

code-review/README.md
+Reviewing code
+--------------
+
+Understand why the code is necessary (bug, user experience, engineering). Then:
@r00k

r00k Jan 7, 2013

Not sure what 'engineering' means in this context.

@croaky

croaky Jan 7, 2013

Contributor

I think I meant "refactoring". Changed: cf21a32. Make more sense?

code-review/README.md
+Understand why the code is necessary (bug, user experience, engineering). Then:
+
+* Communicate which ideas you feel strongly about and those you don't.
+* Don't block code for academic reasons. Object-oriented design and other topics
@r00k

r00k Jan 7, 2013

This part feels odd. I think it makes sense to consider OO design during code review.

It's not quite the place for an in-depth debate, but feedback like "What if we used a Visitor to solve this? It'd would address issue X and Y, at the small cost of tradeoff A" seems valuable.

@gabebw

gabebw Jan 7, 2013

Contributor

I agree with Ben here. I often make comments like his example above.

@croaky

croaky Jan 7, 2013

Contributor

Good feedback.

I wasn't trying to call out OO design specifically here. It was meant as an example. Main point was to move longer-running discussions offline. Have seen threads turn academic or philosophical (is there a better way to say this?) and hold up merging functional code. If teammates can't come to agreement in code review, I'm trying to offer an agreed-upon process for picking a solution and then returning to the discussion at an appropriate time. If the offline discussion results in a new refactoring ticket, that's cool, too.

Took a stab at re-writing here: 693f4d5.

+* Don't use hyperbole. ("always", "never", "endlessly", "nothing")
+* Don't use sarcasm.
+* Keep it real. If emoji, animated gifs, or humor aren't you, don't force them.
+ If they are, use them with aplomb.
@gabebw

gabebw Jan 7, 2013

Contributor

I enjoy the use of "aplomb" here. 👏

Contributor

gabebw commented Jan 7, 2013

I always get at least one person to sign off on a PR before merging it. For example:

  • I submit a PR
  • They have feedback
  • I respond to their feedback and fix all issues
  • They say "Looks good, just fix the typo and merge"

Since I'm still making 1 more change after they signed off on the PR, I still want to get a last 👍 from them. I want the last comment on a PR to be someone saying "Good to merge!" Should that be part of this code review guideline? Do other people even follow this workflow?

croaky added some commits Jan 7, 2013

Clarify extraction of conversation offline
Don't need to call out OO design specifically.
Contributor

croaky commented Jan 7, 2013

I respond to their feedback and fix all issues

They say "Looks good, just fix the typo and merge"

I often make this comment to others. I'll sometimes say "Looks good. Ready to squash and merge after you decide how to handle my feedback." My intent is that the commit looks good enough to merge after their use their judgement on whether more changes are necessary and their judgement whether a re-review will be necessary.

Since I'm still making 1 more change after they signed off on the PR, I still want to get a last from them. I want the last comment on a PR to be someone saying "Good to merge!" Should that be part of this code review guideline? Do other people even follow this workflow?

I'll run some tests or use CI on the branch to let me know it's ready to merge but if the change(s) were small, I won't bother the reviewer with a re-review.

Made a change based on your feedback: 2388fcc. Better?

code-review/README.md
+* Seek to understand the reviewer's perspective.
+* Wait to merge the branch until another person signs off.
+* Wait to merge the branch until a Continuous Integration service like TDDium or
+ TravisCI to tell you the test suite is green in the branch.
@gabebw

gabebw Jan 7, 2013

Contributor

I think this should be "tells" instead of "to tell"? Also, some projects don't have TDDium or Travis set up. What about:

  • If you have Continuous Integration (TDDium, TravisCI) set up, wait to merge the branch until your CI service tells you the test suite is green in the branch.
Contributor

gabebw commented Jan 7, 2013

Made a change based on your feedback: 2388fcc. Better?

Yes, much! I added one comment, but certainly vastly improved. The "phone tag" that can sometimes happen is important to codify, and you've done that.

Contributor

harlow commented Jan 7, 2013

Made a change based on your feedback: 2388fcc. Better?

Very cool. Will these commits still exist once the branch has been squashed?

+* Be grateful for the reviewer's suggestions. ("Good call. I'll make that
+ change.")
+* Don't take it personally. The review is of the code, not you.
+* Explain why the code exists. ("It's like that because of these reasons. Would
@jyurek

jyurek Jan 7, 2013

Member

Should we put something in about trying to respond to every comment? I try to unless it's obvious (to everyone) why I'm not. That way there's more explanation and less of what could look like stonewalling on an issue with the code. Also, it makes sure you see everything.

@jessieay

jessieay Jan 7, 2013

Contributor

👍 on that idea, @jyurek

Owner

jferris commented Jan 11, 2013

I think this provides an excellent base and is good enough that we should merge it at this point, and further suggestions should be submitted as pull requests.

Contributor

gabebw commented Jan 11, 2013

I agree with Joe, we've put Dan through enough :)

Well done Dan, this is very well-written and pithy.

@croaky croaky closed this Jan 11, 2013

Contributor

croaky commented Jan 11, 2013

Thanks, all. Looks like 10 people contributed here. I like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment