Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Get Your Patch Accepted

Get Your Patch Accepted

Code reviews and how to survive them (from Froscon 2015)

Lorna Mitchell

August 22, 2015
Tweet

More Decks by Lorna Mitchell

Other Decks in Technology

Transcript

  1. How to get your patch accepted
    Lorna Mitchell, August 2015

    View Slide

  2. maintainer and team lead

    View Slide

  3. unsolicited advice

    View Slide

  4. stories

    View Slide

  5. 3 ingredients

    View Slide

  6. code

    View Slide

  7. words

    View Slide

  8. perspective

    View Slide

  9. what does it do?

    View Slide

  10. feature/bug

    View Slide

  11. ticket(s)?

    View Slide

  12. If there aren't any
    acceptance criteria for your
    patch, invent some

    View Slide

  13. all the code

    View Slide

  14. git diff master

    View Slide

  15. git diff master...HEAD

    View Slide

  16. tools

    View Slide

  17. TOO BIG

    View Slide

  18. common code mistakes

    View Slide

  19. exotic code, no comments

    View Slide

  20. Leaving commented code in
    your codebase is like leaving
    litter on a campsite

    View Slide

  21. variable named $x

    View Slide

  22. contributing.md

    View Slide

  23. can you break your code?

    View Slide

  24. I can

    View Slide

  25. missing data

    View Slide

  26. permissions

    View Slide

  27. user is an idiot

    View Slide

  28. tests

    View Slide

  29. coding standards

    View Slide

  30. syntax check

    View Slide

  31. The commit history in your
    branch will be part of my
    project until the end of time

    View Slide

  32. git reset --soft $(git merge-base master HEAD)

    View Slide

  33. more info than diff

    View Slide

  34. accurate

    View Slide

  35. sane format

    View Slide

  36. http://chris.beams.io/posts/git-commit/

    View Slide

  37. SFW

    View Slide

  38. apologies

    View Slide

  39. will it merge?

    View Slide

  40. git merge --no-commit --no-ff feature
    git merge --abort

    View Slide

  41. fix it

    View Slide

  42. rebase

    View Slide

  43. A pull request is the opening
    line in a conversation about
    a feature

    View Slide

  44. source/target

    View Slide

  45. diff

    View Slide

  46. title

    View Slide

  47. description

    View Slide

  48. what does it do?

    View Slide

  49. what will I observe?

    View Slide

  50. why do I care?

    View Slide

  51. code review

    View Slide

  52. "review" vs "merge"

    View Slide

  53. anyone

    View Slide

  54. peer review vs gatekeepers

    View Slide

  55. key skills

    View Slide

  56. Being able to see what
    you're NOT looking at is a
    reviewer's superpower

    View Slide

  57. documentation

    View Slide

  58. tests

    View Slide

  59. database patch

    View Slide

  60. deployability

    View Slide

  61. template

    View Slide

  62. email

    View Slide

  63. report

    View Slide

  64. monitoring

    View Slide

  65. cron job

    View Slide

  66. and if not ...

    View Slide

  67. reject

    View Slide

  68. automated build

    View Slide

  69. constructive feedback

    View Slide

  70. colleagues vs contributors

    View Slide

  71. too much feedback

    View Slide

  72. sandwich technique

    View Slide

  73. harsh feedback

    View Slide

  74. Pull requests have a
    lifecycle, be prepared to
    champion them to the end

    View Slide

  75. 3 ingredients

    View Slide

  76. code

    View Slide

  77. words

    View Slide

  78. perspective

    View Slide

  79. Thankyou
    http://lornajane.net
    (also speaking about PHP7 tomorrow)

    View Slide