Pull Requests and Commit Messages

Tips on writing good commit messages and creating easily-reviewable pull requests.

Transcript

1. COMMIT MESSAGES AND PULL REQUESTS by Dana Jones Fractured Atlas
2. twitter.com/commitcampaign
3. BEST PRACTICES Confirm that the work you intend to do is the work that is needed. Remember, a Trello card is a promise of a conversation, not a work order.
4. BEST PRACTICES Confirm that the work you intend to do is the work that is needed. Remember, a Trello card is a promise of a conversation, not a work order. Make sure branch names are dash-separated, and not underscore_separated. As we move more of our infrastructure to Kubernetes, this becomes more important.
5. Set your editor’s preferences to trim trailing whitespace from lines. Variations on this setting muddies the git history and complicates git forensics.
6. Shutterstock.com
7. Susie writes a new feature: def add_numbers(num_1, num_2) Rails.logger("Add some numbers") added = num_1.to_i + num_2.to_i puts added end
8. Susie writes a new feature: def add_numbers(num_1, num_2) Rails.logger("Add some numbers") added = num_1.to_i + num_2.to_i puts added end Martin removes the comment, and adds some trailing whitespace: def add_numbers(num_1, num_2) added = num_1.to_i + num_2.to_i puts added end
9. Susie writes a new feature: def add_numbers(num_1, num_2) Rails.logger("Add some numbers") added = num_1.to_i + num_2.to_i puts added end Martin removes the comment, and adds some trailing whitespace: def add_numbers(num_1, num_2) added = num_1.to_i + num_2.to_i puts added end Anna changes a totally different function in the class, but adds more whitespace: def add_numbers(num_1, num_2) added = num_1.to_i + num_2.to_i puts added end
10. SpongeBob Time Cards
 https://www.youtube.com/channel/UCjfmzjLRyiwgMnZ2pltNSlQ
11. Susie writes a new feature: def add_numbers(num_1, num_2) Rails.logger("Add some numbers") added = num_1.to_i + num_2.to_i puts added end Martin removes the comment, and adds some trailing whitespace: def add_numbers(num_1, num_2) added = num_1.to_i + num_2.to_i puts added end Anna changes a totally different function in the class, but adds more whitespace: def add_numbers(num_1, num_2) added = num_1.to_i + num_2.to_i puts added end Roger is trying to figure out who wrote the add_numbers function so he can ask them about it, and he gets all kinds of misleading information: Anna > def add_numbers(num_1, num_2) Martin > added = num_1.to_i + num_2.to_i Anna > puts added Susie > end
12. Set your editor’s preferences to trim trailing whitespace from lines. Variations on this setting muddies the git history and complicates git forensics. Never use git add . when adding code changes. Use git add -p instead. This protects you from making inadvertent additions to the repo (debugging code, tabs instead of spaces, stray characters, etc.)
13. GIT ADD -P Deletions Additions Whitespace
14. Set your editor’s preferences to trim trailing whitespace from lines. Variations on this setting muddies the git history and complicates git forensics. Never use git add . when adding code changes. Use git add -p instead. This protects you from making inadvertent additions to the repo (debugging code, tabs instead of spaces, stray characters, etc.) The name of an object should indicate what it does. Doing this reduces cognitive load for anyone reviewing your code, and for anyone working on your code in the future.
15. def transact loser.addresses.each do |address| if winner.addresses.blank? address.update_attributes(:person_id => winner.id) elsif !winner.addresses.include?(address) address.update_attributes(:person_id => winner.id, :is_primary => false) end end if winner.addresses.present? && ! winner.addresses.select(&:is_primary?).present? winner.addresses.first.update_attributes(:is_primary => true) end end
16. def transact reassign_addresses set_primary_winner_address end def reassign_addresses loser.addresses.each do |address| if winner_addresses.blank? address.update_attributes(person_id: winner.id) elsif !winner_addresses.include?(address) address.update_attributes(person_id: winner.id, is_primary: false) end end end def set_primary_winner_address if winner_addresses.present? && ! winner_addresses.select(&:is_primary?).present? winner_addresses.first.update_attributes(is_primary: true) end end
17. COMMIT MESSAGES
18. Commit messages should make sense in isolation. They should tell the what and the why.
 
 Examples that do not achieve this: Respond to feedback
19. Commit messages should make sense in isolation. They should tell the what and the why.
 
 Examples that do not achieve this: Respond to feedback Fix spec
20. Commit messages should make sense in isolation. They should tell the what and the why.
 
 Examples that do not achieve this: Respond to feedback Fix spec Make CodeClimate happy
21. Commit messages should make sense in isolation. They should tell the what and the why.
 
 Examples that do not achieve this: Respond to feedback Fix spec Make CodeClimate happy Add logging
22. Commit messages should make sense in isolation. They should tell the what and the why.
 
 Examples that do not achieve this: Respond to feedback Fix spec Make CodeClimate happy Add logging Clean up
23. Commit messages should make sense in isolation. They should tell the what and the why.
 
 Examples that do not achieve this: Respond to feedback Fix spec Make CodeClimate happy Add logging Clean up Abc order
24. Commit messages should make sense in isolation. They should tell the what and the why.
 
 Examples that do not achieve this: Respond to feedback Fix spec Make CodeClimate happy Add logging Clean up Abc order God doesn't love ugly
25. https://revelry.co/pull-requests-code-review/
26. WHAT IS A PULL REQUEST? A Pull Request (PR) is a formalized request that a project maintainer add the submitted code changes to the project repository. Unless otherwise flagged, a PR is a signal to a project
 maintainer and other contributors to the project that the
 code it includes is production-ready.
27. CODE THAT ISN’T PRODUCTION-READY If you just want someone to review work in progress, push the branch you’re working on, and ask the reviewer to check it out. Alternatively, you can create a PR so the reviewer can easily review and comment on the work, but make sure you add the WIP label.
28. EFFECTIVE PULL REQUESTS Smaller PRs are easier and faster to review. Create iterative PRs for large features whenever possible. If you find this hard to do, think about taking a large feature branch and figuring out how you could break it up and re-create it in smaller, more digestible chunks that still get the same result.
29. PR DESCRIPTION express your intent, link to the Trello card, include contextual information, warn about any “gotchas,” give instructions for testing and deploying, avoid accusatory or defensive language, and include a screenshot or animated GIF to demonstrate UX changes. Or just for fun. :) A PR description should:
30. REVIEWING PULL REQUESTS
31. WHAT TO LOOK FOR Do automatic checks pass? For those that don’t, are the failures legitimate? If so, notify the submitter and pause review.
32. WHAT TO LOOK FOR Do automatic checks pass? For those that don’t, are the failures legitimate? If so, notify the submitter and pause review. Are there files with conflicts? If so, these should be resolved before the review proceeds, because files will change.
33. WHAT TO LOOK FOR Do automatic checks pass? For those that don’t, are the failures legitimate? If so, notify the submitter and pause review. Are there files with conflicts? If so, these should be resolved before the review proceeds, because files will change. Does the PR have migrations? If so, part of your review should be to run rake db:migrate and rake db:rollback to ensure there are no syntax errors.
34. Are there any missing new files? It’s a common mistake to forget to add view files, support files (eg. test import files), images, etc. to the git repo.
35. Are there any missing new files? It’s a common mistake to forget to add view files, support files (eg. test import files), images, etc. to the git repo. Examine the fallout of deletions - were all references to a CSS class removed, but the class itself persisted? Same for JS functions, view partials, functions, I18n locale keys, routes, etc.
36. Are there any missing new files? It’s a common mistake to forget to add view files, support files (eg. test import files), images, etc. to the git repo. Examine the fallout of deletions - were all references to a CSS class removed, but the class itself persisted? Same for JS functions, view partials, functions, I18n locale keys, routes, etc. Are there sufficient tests to cover the functionality that's been added?
37. Are there any missing new files? It’s a common mistake to forget to add view files, support files (eg. test import files), images, etc. to the git repo. Examine the fallout of deletions - were all references to a CSS class removed, but the class itself persisted? Same for JS functions, view partials, functions, I18n locale keys, routes, etc. Are there sufficient tests to cover the functionality that's been added? Are there any public functions that should be made private?
38. LEAVING REVIEW COMMENTS Apply the principles explored in the book Radical Candor by Kim Scott - feedback should be both personal and direct (see Ian Feather’s blog post).
39. LEAVING REVIEW COMMENTS Apply the principles explored in the book Radical Candor by Kim Scott - feedback should be both personal and direct (see Ian Feather’s blog post). Feedback on a PR can be either positive or negative, but should be meaningful.
40. LEAVING REVIEW COMMENTS Apply the principles explored in the book Radical Candor by Kim Scott - feedback should be both personal and direct (see Ian Feather’s blog post). Feedback on a PR can be either positive or negative, but should be meaningful. Phrasing critical feedback as questions rather than directives is gentler and less combative.
41. "Have you considered...?" "I wonder if it might be helpful to..." "What do you think about...?" "Can you clarify...?" "Why didn't you just...?"
42. weheartit.com/entry/2454490
43. "Have you considered...?" "I wonder if it might be helpful to..." "What do you think about...?" "Can you clarify...?" "Why didn't you just...?"
44. Ask clarifying questions if there’s something you don’t understand. You learn, and you aren’t rubber- stamping code that could be problematic.
45. Ask clarifying questions if there’s something you don’t understand. You learn, and you aren’t rubber- stamping code that could be problematic. If the feedback you need to give is pervasive or potentially embarrassing, consider talking to the submitter directly instead of putting the feedback in writing.
46. Ask clarifying questions if there’s something you don’t understand. You learn, and you aren’t rubber- stamping code that could be problematic. If the feedback you need to give is pervasive or potentially embarrassing, consider talking to the submitter directly instead of putting the feedback in writing. Ask yourself if your critical comment/inquiry is adding real value or just picking a nit.
47. If you have a considerable number of suggestions for changes, think about creating a branch from the PR branch, make your changes, and submitting a PR to the submitter’s branch.